feat(diagnostics): add trace context carrier#70924
Conversation
Greptile SummaryThis PR adds a lightweight W3C-compatible diagnostic trace context carrier ( Confidence Score: 5/5Safe to merge; all remaining concerns are P2 style/edge-case notes, and prior P1 threads (random-ID zero-check, future-version traceparent) have already been surfaced to the author. The core implementation is correct: byte counts in randomHex are right (16 bytes → 32 hex for traceId, 8 bytes → 16 hex for spanId), normalization and validation guards are consistent, the self-parenting check works as tested, and no global state is added. No new P0 or P1 issues were found beyond the two already commented in prior threads. No files require special attention. Reviews (2): Last reviewed commit: "test(agents): follow split MiniMax FAQ d..." | Re-trigger Greptile |
| const traceId = normalizeTraceId(input.traceId) ?? parsed?.traceId ?? randomHex(16); | ||
| const spanId = normalizeSpanId(input.spanId) ?? parsed?.spanId ?? randomHex(8); |
There was a problem hiding this comment.
randomHex(16) and randomHex(8) produce raw hex strings that skip the isNonZeroHex guard applied by normalizeTraceId/normalizeSpanId. While the probability of crypto.randomBytes generating all-zero bytes is negligible (~2⁻¹²⁸ / 2⁻⁶⁴), if it did happen the context object would contain IDs that fail isValidDiagnosticTraceId/isValidDiagnosticSpanId and formatDiagnosticTraceparent would silently return undefined. A defensive wrapper makes the invariant explicit:
function randomTraceId(): string {
let id: string;
do { id = randomHex(16); } while (!isNonZeroHex(id));
return id;
}
function randomSpanId(): string {
let id: string;
do { id = randomHex(8); } while (!isNonZeroHex(id));
return id;
}Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/diagnostic-trace-context.ts
Line: 111-112
Comment:
**Random ID fallback bypasses non-zero validation**
`randomHex(16)` and `randomHex(8)` produce raw hex strings that skip the `isNonZeroHex` guard applied by `normalizeTraceId`/`normalizeSpanId`. While the probability of `crypto.randomBytes` generating all-zero bytes is negligible (~2⁻¹²⁸ / 2⁻⁶⁴), if it did happen the context object would contain IDs that fail `isValidDiagnosticTraceId`/`isValidDiagnosticSpanId` and `formatDiagnosticTraceparent` would silently return `undefined`. A defensive wrapper makes the invariant explicit:
```ts
function randomTraceId(): string {
let id: string;
do { id = randomHex(16); } while (!isNonZeroHex(id));
return id;
}
function randomSpanId(): string {
let id: string;
do { id = randomHex(8); } while (!isNonZeroHex(id));
return id;
}
```
How can I resolve this? If you propose a fix, please make it concise.| traceparent: string | undefined, | ||
| ): DiagnosticTraceContext | undefined { | ||
| const parts = traceparent?.trim().toLowerCase().split("-"); | ||
| if (!parts || parts.length !== 4) { |
There was a problem hiding this comment.
The W3C Trace Context spec (§2.2) requires that implementations parsing a traceparent with an unknown version still attempt to extract traceId and parentId from the first four dash-delimited fields and continue the trace. The current strict parts.length !== 4 guard and the version !== "00" rejection together discard any header with a future version number without attempting extraction. This is acceptable for an internal carrier now, but is worth documenting if parseDiagnosticTraceparent will later be used to accept inbound headers from external callers.
| if (!parts || parts.length !== 4) { | |
| if (!parts || parts.length < 4) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/diagnostic-trace-context.ts
Line: 72
Comment:
**W3C spec: future-version traceparents silently dropped**
The W3C Trace Context spec (§2.2) requires that implementations parsing a traceparent with an unknown version still attempt to extract `traceId` and `parentId` from the first four dash-delimited fields and continue the trace. The current strict `parts.length !== 4` guard and the `version !== "00"` rejection together discard any header with a future version number without attempting extraction. This is acceptable for an internal carrier now, but is worth documenting if `parseDiagnosticTraceparent` will later be used to accept inbound headers from external callers.
```suggestion
if (!parts || parts.length < 4) {
```
How can I resolve this? If you propose a fix, please make it concise.c532f74 to
efe515d
Compare
efe515d to
d3db4e4
Compare
d3db4e4 to
b5e1671
Compare
🔒 Aisle Security AnalysisWe found 1 potential security issue(s) in this PR:
1. 🟡 Potential DoS via unbounded `traceparent` header parsing
DescriptionThe
An attacker can send an extremely large Vulnerable code: const parts = traceparent?.trim().toLowerCase().split("-");RecommendationAdd a strict maximum length check before any string transformations (the W3C Example: const MAX_TRACEPARENT_LEN = 128; // generous upper bound
export function parseDiagnosticTraceparent(traceparent?: string) {
if (typeof traceparent !== "string") return undefined;
if (traceparent.length > MAX_TRACEPARENT_LEN) return undefined;
const parts = traceparent.trim().toLowerCase().split("-");
// ... existing validation ...
}Optionally, avoid Analyzed PR: #70924 at commit Last updated on: 2026-04-24T05:15:04Z |
b5e1671 to
1bd7a06
Compare
Summary
traceon diagnostic events, plugin-sdk diagnostics exports, focused tests, and the plugin SDK baseline update.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
N/A
Regression Test Plan (if applicable)
N/A
src/infra/diagnostic-trace-context.test.ts,src/infra/diagnostic-events.test.tsUser-visible / Behavior Changes
Adds plugin-sdk diagnostics helpers that bundled diagnostics-otel and future observability plugins can use for trace correlation. No runtime telemetry behavior changes yet.
Diagram (if applicable)
Security Impact (required)
No)No)No)No)No)Yes, explain risk + mitigation:Repro + Verification
Environment
Steps
pnpm test src/infra/diagnostic-events.test.tspnpm test src/infra/diagnostic-trace-context.test.tspnpm format:check -- CHANGELOG.md docs/.generated/plugin-sdk-api-baseline.sha256 src/infra/diagnostic-trace-context.ts src/infra/diagnostic-trace-context.test.ts src/infra/diagnostic-events.ts src/infra/diagnostic-events.test.ts src/plugin-sdk/diagnostics-otel.tspnpm plugin-sdk:api:checkpnpm check:changedorigin/main:pnpm tsgo:coreExpected
check:changedshould get past local change checks; current unrelated main typecheck issue may still fail.Actual
pnpm plugin-sdk:api:checkpassed.pnpm check:changedfailed attsgo:corewith unrelated existing errors insrc/agents/openai-transport-stream.tsandsrc/plugin-sdk/provider-tools.ts.pnpm tsgo:corefailure reproduces on latestorigin/main.Evidence
Human Verification (required)
What you personally verified (not just CI), and how:
Review Conversations
Compatibility / Migration
Yes)No)No)Risks and Mitigations