fix(span): correctly parse multi-segment trace targets in single-arg view#1267
fix(span): correctly parse multi-segment trace targets in single-arg view#1267sentry[bot] wants to merge 1 commit into
Conversation
|
![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 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2dc800b. Configure here.
| `Use separate arguments: sentry span view ${traceArg} ${maybeSpanId}` | ||
| ); | ||
| return { kind: "deferred", rawTraceArg: traceArg, rawSpanIds: [maybeSpanId] }; | ||
| } |
There was a problem hiding this comment.
Prefix missing trace ID accepted
Medium Severity
The new single-argument splitter treats any path whose prefix still contains / and whose final segment is 16-character hex as <trace-target>/<span-id>. A three-segment input like org/project/<span-id> (trace target only, no separate span argument) is split into rawTraceArg org/project and a span ID, instead of the prior missing-span ContextError. Trace resolution then targets the wrong path.
Reviewed by Cursor Bugbot for commit 2dc800b. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| log.warn( | ||
| `Interpreting '${first}' as <trace-target>/<span-id>. ` + | ||
| `Use separate arguments: sentry span view ${traceArg} ${maybeSpanId}` | ||
| ); | ||
| return { kind: "deferred", rawTraceArg: traceArg, rawSpanIds: [maybeSpanId] }; |
There was a problem hiding this comment.
Bug: The warning for combined arguments like org/project/traceId/spanId shows a normalized span ID (dashes removed), which can be confusing as it differs from the user's original input.
Severity: LOW
Suggested Fix
To avoid user confusion, the warning message should display the original, un-normalized span ID as entered by the user. The normalized version of the span ID should still be used internally for validation and further processing, but the user-facing output should reflect their exact input.
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/span/view.ts#L145-L149
Potential issue: When a user provides a combined argument like
`org/project/traceId/spanId`, the code normalizes the span ID by removing dashes and
lowercasing it. This normalized ID is then displayed in a warning message that suggests
the correct command usage. If the original span ID contained dashes, the suggested
command will show a span ID without dashes, which differs from the user's input. This
can be confusing for the user, even though the normalized ID is functionally valid for
subsequent processing. This new logic path is also not covered by tests.
Did we get this right? 👍 / 👎 to inform future reviews.
Codecov Results 📊❌ Patch coverage is 71.43%. Project has 5482 uncovered lines. Files with missing lines (1)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 81.67% 81.67% —%
==========================================
Files 426 426 —
Lines 29896 29903 +7
Branches 19420 19428 +8
==========================================
+ Hits 24416 24421 +5
- Misses 5480 5482 +2
- Partials 2035 2036 +1Generated by Codecov Action |
1 participant
The
sentry span viewcommand'sparsePositionalArgsfunction previously only supported auto-splitting a single positional argument if it was in the<trace-id>/<span-id>format (i.e., exactly one slash). When users provided a fully-qualified target like<org>/<project>/<trace-id>/<span-id>as a single argument, the parser would skip the auto-split logic due to the multiple slashes. This resulted in therawSpanIdsarray being empty, leading to aContextError: Span ID is required.This fix extends the
parsePositionalArgsfunction to correctly handle single arguments that contain multiple slashes, specifically looking for the<org>/<project>/<trace-id>/<span-id>pattern. It now extracts the span ID from the last segment and treats the preceding part (<org>/<project>/<trace-id>) as the trace target, which is then processed by the existingparseTraceTargetWithRecoverymechanism. A warning message is also logged to advise users to use separate arguments for clarity.Fixes CLI-1BD
Comment
@sentry <feedback>on this PR to have Autofix iterate on the changes.