feat: sourcemap upload parity flags and issue archive command#883
feat: sourcemap upload parity flags and issue archive command#883
Conversation
Add missing sourcemap flags for old sentry-cli parity: - --dist: distribution identifier for builds within a release - --ext: custom file extensions (passthrough from inject to upload) - --no-rewrite: upload files without injecting debug IDs - --ignore/--ignore-file: gitignore-style file exclusion - --strip-prefix/--strip-common-prefix: path prefix removal Add sourceMappingURL directive following in inject: - Reads last 512 bytes of JS files to find //# sourceMappingURL= - Resolves relative paths when convention naming (foo.js.map) fails - Gracefully skips data: and http: URLs Add issue archive command (sentry issue archive, alias: ignore): - Maps to Sentry 'ignored' status via existing updateIssueStatus() - Supports --duration, --count, --window, --users, --user-window for conditional archiving Closes #600 (sourcemap and issue mutation gaps)
|
Codecov Results 📊✅ 6311 passed | Total: 6311 | Pass Rate: 100% | Execution Time: 0ms 📊 Comparison with Base Branch
All tests are passing successfully. ❌ Patch coverage is 75.53%. Project has 13197 uncovered lines. Files with missing lines (6)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
- Coverage 75.97% 75.91% -0.06%
==========================================
Files 294 295 +1
Lines 54402 54785 +383
Branches 0 0 —
==========================================
+ Hits 41329 41588 +259
- Misses 13073 13197 +124
- Partials 0 0 —Generated by Codecov Action |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| const diag = await diagnoseEmptyDiscovery(dir, { extensions }); | ||
| throw buildEmptyDiscoveryError(dir, diag); | ||
| } | ||
|
|
||
| const results = await injectDirectory(dir, { | ||
| extensions, | ||
| ignorePatterns, | ||
| dryRun: flags["dry-run"], | ||
| }); |
There was a problem hiding this comment.
Bug: The inject command fails to pass the ignoreMatcher to injectDirectory, causing files specified in --ignore-file to be incorrectly processed during injection.
Severity: MEDIUM
Suggested Fix
Pass the fully constructed ignoreMatcher from the inject command to the injectDirectory function via the options.ignoreMatcher property, instead of only passing ignorePatterns.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/commands/sourcemap/inject.ts#L155-L163
Potential issue: The `inject` command correctly builds an `ignoreMatcher` using patterns
from both the `--ignore` flag and the `--ignore-file` flag. This matcher is used for an
initial file discovery. However, when calling the `injectDirectory` function, only the
`ignorePatterns` from the `--ignore` flag are passed. The `injectDirectory` function
then rebuilds the ignore matcher without the contents of the file specified by
`--ignore-file`. As a result, files that should be ignored via the ignore file are
incorrectly processed and have debug IDs injected.
Did we get this right? 👍 / 👎 to inform future reviews.
| */ | ||
| async function findCompanionMap(jsPath: string): Promise<string | undefined> { | ||
| const mapPath = `${jsPath}.map`; | ||
| const SOURCE_MAPPING_URL_RE = /\/\/[#@]\s*sourceMappingURL\s*=\s*(\S+)\s*$/m; |
There was a problem hiding this comment.
Bug: The regex for finding sourceMappingURL uses a multiline flag that can cause it to match the first occurrence in a file's tail, not the last, potentially picking up incorrect URLs from string literals.
Severity: LOW
Suggested Fix
Modify the SOURCE_MAPPING_URL_RE to ensure it only matches the final sourceMappingURL directive. This can be done by removing the multiline (m) flag so $ only matches the end of the string, or by using the global (g) flag and taking the last result.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/lib/sourcemap/inject.ts#L95
Potential issue: The regex `SOURCE_MAPPING_URL_RE` is intended to find the source
mapping URL comment, which should be the last line in a file. However, it uses the
multiline (`m`) flag, causing `$` to match the end of any line. When combined with
`String.match()`, which returns the first match, it can incorrectly extract a URL from a
string literal if it appears before the actual final `sourceMappingURL` comment in the
last 512 bytes of a file. This violates the documented intent to match only the final
directive.
Did we get this right? 👍 / 👎 to inform future reviews.
| let pathPrefixToStrip = flags["strip-prefix"] ?? ""; | ||
| if (flags["strip-common-prefix"]) { | ||
| const allRelative = results.flatMap(({ jsPath, mapPath }) => [ | ||
| relative(resolvedDir, jsPath).replaceAll("\\", "/"), | ||
| relative(resolvedDir, mapPath).replaceAll("\\", "/"), | ||
| ]); | ||
| pathPrefixToStrip = computeCommonPrefix(allRelative); |
There was a problem hiding this comment.
Bug: When both --strip-prefix and --strip-common-prefix flags are used, the value from --strip-prefix is silently overwritten, leading to unexpected behavior.
Severity: MEDIUM
Suggested Fix
Implement logic to handle the case where both flags are provided. Either throw a validation error to indicate they are mutually exclusive, or establish a clear precedence (e.g., strip-prefix should override strip-common-prefix) and document this behavior.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/commands/sourcemap/upload.ts#L287-L293
Potential issue: When both the `--strip-prefix` and `--strip-common-prefix` flags are
used with the `upload` command, the value from `--strip-prefix` is silently overwritten
by the value computed for `--strip-common-prefix`. There is no validation to prevent
using both flags, nor is there a warning that the explicitly provided prefix is being
ignored. This undocumented behavior can lead to unexpected artifact paths in Sentry, as
the user's explicit configuration is discarded without their knowledge.
Did we get this right? 👍 / 👎 to inform future reviews.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0970403. Configure here.
|
|
||
| const results = await injectDirectory(dir, { | ||
| extensions, | ||
| ignorePatterns, |
There was a problem hiding this comment.
Inject command drops --ignore-file patterns during injection
High Severity
The inject command builds an ignoreMatcher from both --ignore patterns and --ignore-file, and correctly uses it for the discoverFilePairs pre-check. But it then passes only ignorePatterns (not ignoreMatcher) to injectDirectory. Inside injectDirectory, the matcher is rebuilt from just ignorePatterns, losing all patterns from --ignore-file. This makes --ignore-file silently ineffective during actual injection. The upload command correctly passes ignoreMatcher to injectDirectory; the inject command needs to do the same.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 0970403. Configure here.
| try { | ||
| const tail = await readFileTail(jsPath); | ||
| const match = tail.match(SOURCE_MAPPING_URL_RE); | ||
| return match?.[1]; |
There was a problem hiding this comment.
Regex returns first sourceMappingURL match, not last
Low Severity
The source map spec says the last sourceMappingURL directive in a file is authoritative. extractSourceMappingUrl uses tail.match(SOURCE_MAPPING_URL_RE) which returns the first match in the 512-byte tail. If the tail contains multiple directives (e.g. concatenated bundles), the wrong sourcemap could be paired. The regex needs the g flag with iteration to find the last match, or a reverse search approach.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 0970403. Configure here.
|
Splitting into two PRs: one for sourcemap enhancements, one for issue archive. |
1 participant
Summary
Closes #600 — implements Phase 1 of the feature parity plan (sourcemap enhancements + issue archive).
Sourcemap upload: 6 new flags for old sentry-cli parity
--dist— Distribution identifier to disambiguate builds within a release. Passed through to the artifact bundle manifest and assemble endpoint.--ext— Custom file extensions on upload (was only available oninject). Forwards to the discovery/inject flow.--no-rewrite— Upload files as-is without injecting debug IDs, relying on release/URL-based matching instead.--ignore/--ignore-file— Gitignore-style file exclusion patterns on bothinjectandupload. Uses theignorenpm package (already in devDependencies).--strip-prefix— Remove an explicit prefix from uploaded file URLs.--strip-common-prefix— Auto-detect and strip the longest common directory prefix from all files.Sourcemap inject:
sourceMappingURLdirective followingThe inject command's companion map discovery was previously strictly filename-based (
foo.js→foo.js.map). Now it also parses//# sourceMappingURL=<url>directives from JS files:sourceMappingURLfallbackdata:(inline) andhttp:/https:(remote) URLs with debug loggingNew command:
sentry issue archive(alias:ignore)Archives (ignores) an issue, suppressing alerts until an optional condition is met:
--duration— Ignore for N minutes--count/--window— Ignore until N events in M minutes--users/--user-window— Ignore until N users affected in M minutesMaps to the existing
updateIssueStatus("ignored")API. Registered withignoreas an alias.Tests