fix(diagnostics): harden trace context parsing by vincentkoc · Pull Request #70955 · openclaw/openclaw

vincentkoc · 2026-04-24T05:41:36Z

Summary

Problem: follow-up review on the diagnostic trace context carrier found two edge cases: random fallback ids did not make the non-zero invariant explicit, and future-version traceparents were dropped.
Why it matters: invalid fallback ids could break later traceparent formatting in the impossible all-zero RNG case, and future W3C traceparent versions should still preserve correlation where possible.
What changed: added non-zero random trace/span id wrappers; parse future non-ff traceparent versions from the first four fields while keeping version 00 strict.
What did NOT change (scope boundary): no OTEL SDK wiring, no global state, no log/hook/event fanout changes.

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Related feat(diagnostics): add trace context carrier #70924
This PR fixes a bug or regression

Root Cause (if applicable)

Root cause: generated fallback ids came directly from randomBytes().toString("hex"), bypassing the same non-zero validation path used for inbound ids; parsing also treated all non-00 traceparent versions as invalid instead of preserving the first four fields for future versions.
Missing detection / guardrail: trace context tests did not assert generated fallback ids validate, or future-version traceparent continuation.
Contributing context (if known): initial foundation slice was intentionally small and internal-only.

Regression Test Plan (if applicable)

Coverage level that should have caught this:
- Unit test
- Seam / integration test
- End-to-end test
- Existing coverage already sufficient
Target test or file: src/infra/diagnostic-trace-context.test.ts
Scenario the test should lock in: fallback contexts generate valid non-zero ids, version 00 remains strict, and future non-ff versions continue from the first four fields.
Why this is the smallest reliable guardrail: the behavior is fully contained in the pure trace-context helper.
Existing test that already covers this (if any): N/A
If no new test is added, why not: N/A

User-visible / Behavior Changes

None.

Diagram (if applicable)

N/A

Security Impact (required)

New permissions/capabilities? No
Secrets/tokens handling changed? No
New/changed network calls? No
Command/tool execution surface changed? No
Data access scope changed? No
If any Yes, explain risk + mitigation: N/A

Repro + Verification

Environment

OS: macOS
Runtime/container: local Node/pnpm repo worktree
Model/provider: N/A
Integration/channel (if any): N/A
Relevant config (redacted): N/A

Steps

Run pnpm test src/infra/diagnostic-trace-context.test.ts.
Run pnpm format:check -- src/infra/diagnostic-trace-context.ts src/infra/diagnostic-trace-context.test.ts.
Run pnpm exec oxlint src/infra/diagnostic-trace-context.ts src/infra/diagnostic-trace-context.test.ts.

Expected

Trace context tests pass.
Formatting passes.
Oxlint reports no warnings or errors.

Actual

All listed checks passed locally.

Evidence

Failing test/log before + passing after
Trace/log snippets
Screenshot/recording
Perf numbers (if relevant)

Human Verification (required)

Verified scenarios: future traceparent parsing, strict version 00 extra-field rejection, generated fallback ids validate and format.
Edge cases checked: invalid ff version, all-zero inbound trace/span ids, invalid trace flags.
What you did not verify: full repo-wide CI locally; PR CI will cover broader lanes.

Review Conversations

I replied to or resolved every bot review conversation I addressed in this PR.
I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

Backward compatible? Yes
Config/env changes? No
Migration needed? No
If yes, exact upgrade steps: N/A

Risks and Mitigations

Risk: accepting future traceparent versions could preserve malformed future fields.
- Mitigation: only non-ff two-hex versions are accepted, traceId, spanId, and traceFlags still use the existing validators, and version 00 remains strict at exactly four fields.

greptile-apps · 2026-04-24T05:43:02Z

Greptile Summary

This PR hardens trace context parsing by (1) wrapping randomBytes calls in retry loops that guarantee non-zero trace/span ids, and (2) accepting future W3C traceparent versions (anything except ff) by extracting only the first four fields while keeping version 00 strictly four-field. The logic correctly mirrors the W3C Trace Context spec's forward-compatibility guidance, all three new test cases cover the intended invariants, and no existing behaviour is regressed.

Confidence Score: 5/5

Safe to merge — changes are self-contained, well-tested, and spec-compliant.

All findings are P2 (style/clarity only). The non-zero retry loops are provably correct (probability of an all-zero 128-bit id is 1/2^128), the future-version parsing logic matches W3C spec, and the new tests lock in every stated invariant. No data integrity, security, or runtime correctness concerns.

No files require special attention.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: src/infra/diagnostic-trace-context.ts
Line: 7-8

Comment:
**Duplicate regex constant**

`TRACEPARENT_VERSION_RE` is identical to `TRACE_FLAGS_RE` (`/^[0-9a-f]{2}$/`). Both constants match the same two-hex-digit pattern; using separate names is fine for readability, but a comment noting the intentional duplication (or a reference to the shared shape) would clarify this isn't an oversight.

```suggestion
const TRACE_FLAGS_RE = /^[0-9a-f]{2}$/;
/** Same shape as TRACE_FLAGS_RE; kept separate for semantic clarity. */
const TRACEPARENT_VERSION_RE = /^[0-9a-f]{2}$/;
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "fix(diagnostics): harden trace context p..." | Re-trigger Greptile}

greptile-apps · 2026-04-24T05:43:06Z

 const TRACE_FLAGS_RE = /^[0-9a-f]{2}$/;
+const TRACEPARENT_VERSION_RE = /^[0-9a-f]{2}$/;


Duplicate regex constant

TRACEPARENT_VERSION_RE is identical to TRACE_FLAGS_RE (/^[0-9a-f]{2}$/). Both constants match the same two-hex-digit pattern; using separate names is fine for readability, but a comment noting the intentional duplication (or a reference to the shared shape) would clarify this isn't an oversight.

Suggested change

const TRACE_FLAGS_RE = /^[0-9a-f]{2}$/;

const TRACEPARENT_VERSION_RE = /^[0-9a-f]{2}$/;

const TRACE_FLAGS_RE = /^[0-9a-f]{2}$/;

/** Same shape as TRACE_FLAGS_RE; kept separate for semantic clarity. */

const TRACEPARENT_VERSION_RE = /^[0-9a-f]{2}$/;

Prompt To Fix With AI

This is a comment left during a code review. Path: src/infra/diagnostic-trace-context.ts Line: 7-8 Comment: **Duplicate regex constant** `TRACEPARENT_VERSION_RE` is identical to `TRACE_FLAGS_RE` (`/^[0-9a-f]{2}$/`). Both constants match the same two-hex-digit pattern; using separate names is fine for readability, but a comment noting the intentional duplication (or a reference to the shared shape) would clarify this isn't an oversight. ```suggestion const TRACE_FLAGS_RE = /^[0-9a-f]{2}$/; /** Same shape as TRACE_FLAGS_RE; kept separate for semantic clarity. */ const TRACEPARENT_VERSION_RE = /^[0-9a-f]{2}$/; ``` How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

openclaw-barnacle Bot added the size: XS label Apr 24, 2026

vincentkoc self-assigned this Apr 24, 2026

openclaw-barnacle Bot added the maintainer Maintainer-authored PR label Apr 24, 2026

greptile-apps Bot reviewed Apr 24, 2026

View reviewed changes

fix(diagnostics): harden trace context parsing

cf0ff21

vincentkoc force-pushed the fix/diagnostic-trace-context-hardening branch from 7438da9 to cf0ff21 Compare April 24, 2026 05:44

openclaw-barnacle Bot added size: S and removed size: XS labels Apr 24, 2026

vincentkoc merged commit 350d322 into main Apr 24, 2026
66 checks passed

vincentkoc deleted the fix/diagnostic-trace-context-hardening branch April 24, 2026 05:47

vincentkoc mentioned this pull request Apr 24, 2026

feat(diagnostics): attach trace context to otel logs #70961

Merged

2 tasks

github-actions Bot mentioned this pull request Apr 24, 2026

📡 Upstream Digest — 2026-04-24 07:50 UTC curtismercier/openclaw-mods#664

Open

Angfr95 pushed a commit to Angfr95/openclaw that referenced this pull request Apr 25, 2026

fix(diagnostics): harden trace context parsing (openclaw#70955)

74b07a5

ogt-redknie pushed a commit to ogt-redknie/OPENX that referenced this pull request May 2, 2026

fix(diagnostics): harden trace context parsing (openclaw#70955)

4c66f4e

github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request May 9, 2026

fix(diagnostics): harden trace context parsing (openclaw#70955)

89ada41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(diagnostics): harden trace context parsing#70955

fix(diagnostics): harden trace context parsing#70955
vincentkoc merged 1 commit intomainfrom
fix/diagnostic-trace-context-hardening

vincentkoc commented Apr 24, 2026

Uh oh!

greptile-apps Bot commented Apr 24, 2026

Uh oh!

greptile-apps Bot Apr 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		const TRACE_FLAGS_RE = /^[0-9a-f]{2}$/;
		const TRACEPARENT_VERSION_RE = /^[0-9a-f]{2}$/;

Uh oh!

Conversation

vincentkoc commented Apr 24, 2026

Summary

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Root Cause (if applicable)

Regression Test Plan (if applicable)

User-visible / Behavior Changes

Diagram (if applicable)

Security Impact (required)

Repro + Verification

Environment

Steps

Expected

Actual

Evidence

Human Verification (required)

Review Conversations

Compatibility / Migration

Risks and Mitigations

Uh oh!

greptile-apps Bot commented Apr 24, 2026

Greptile Summary

Confidence Score: 5/5

Uh oh!

greptile-apps Bot Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant