fix(#2779): respect inbound TRACEPARENT in trace chain

[dhshah13]

Closes #2779.

Re-land of #2833, which was closed unmerged for process reasons (unintended auto-code), not substance. The adoption logic now lives in an extracted pure helper (resolveTraceIdentity) so it is unit-tested outside runAgent — addressing the patch-coverage gap that #2833 hit.

What changed

• A valid inbound TRACEPARENT is adopted: its trace-id becomes the unified security/W3C trace id, a fresh root span continues the parent trace, and the W3C trace-flags (sampled bit) are preserved end-to-end. Invalid or absent values fall back to fresh Level 1 identity generation.
• childScriptEnv filters any inherited TRACEPARENT before appending fullsend's own — env lookups resolve the first match, so the stale parent value used to shadow it. TRACESTATE passes through untouched.
• Shell-safety validation at injectTraceID/buildScanContextCommand switches to IsShellSafeTraceID (lowercase hex + dashes in UUID shape): adopted trace ids are not UUID v4, and the charset is what shell interpolation safety depends on.

Beyond #2833

• The root span records the inbound span-id as its remote parent — without this, exported Level 2 traces would orphan from the parent chain.
• run-summary.json's traceparent and Recorder.TraceParent() carry the real flags (previously hardcoded -01, which would re-advertise an upstream-unsampled trace as sampled).
• ParseTraceParent is W3C forward-compatible: version 00 requires exactly four fields, unknown versions tolerate trailing fields, ff/all-zero ids/uppercase are rejected.

Trust note

An upstream process can now choose the run's security trace id via TRACEPARENT. This affects audit correlation, not audit integrity (the finding hash chain covers content, not id uniqueness), and TRACEPARENT arrives from the same trust domain as the rest of the workflow env. fullsend.work_item_id remains the primary cross-run correlation key per ADR 0050.

Testing

• Every new/changed function at 100% statement coverage (resolveTraceIdentity, childScriptEnv, ParseTraceParent, TraceParentWithFlags, UUIDFromTraceID, IsShellSafeTraceID, recorder New/TraceParent/Finalize).
• go test -race green for internal/telemetry, internal/security, internal/cli; gofmt/go vet/pre-commit clean.
• Level 2 (feat: implement Level 2 distributed tracing — OTLP export to a backend #2862) builds directly on the artifacts this produces (remote parent + flags in the JSONL/summary).

[github-actions]

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

[qodo-code-review]

PR Summary by Qodo

Respect inbound TRACEPARENT across the run’s trace chain

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Adopt valid inbound TRACEPARENT to continue the parent trace and preserve sampling flags.
• Filter inherited TRACEPARENT when building child-script env so fullsend’s value wins.
• Relax shell-safety validation to accept adopted (non-UUIDv4) dashed-hex trace IDs.

Diagram

graph TD
  A["runAgent()"] --> B["resolveTraceIdentity()"] --> C{"Valid inbound TRACEPARENT?"}
  C -->|"yes"| D["telemetry.ParseTraceParent()"] --> E["TraceContext (adopt)"] --> F["telemetry.Recorder"] --> G["childScriptEnv()"] --> H["pre/post scripts"]
  C -->|"no"| I["security.GenerateTraceID()"] --> J["TraceContext (fresh)"] --> F
  F --> K["run-summary.json (traceparent+flags)"]
  A --> L["security.IsShellSafeTraceID()"] --> M["shell cmds (.env/context)"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use OpenTelemetry propagator/parser instead of custom ParseTraceParent

• ➕ Avoids maintaining W3C parsing edge-cases in-house
• ➕ Likely already handles forward-compat/versioning rules
• ➖ Adds/expands dependency surface for a small parsing need
• ➖ May pull in heavier APIs than desired for a CLI tool

2. Keep security trace-id independent from W3C trace-id (store a mapping)

• ➕ Avoids letting upstream TRACEPARENT influence the security trace-id
• ➕ Keeps security correlation semantics fully internal
• ➖ Breaks the “one unified id” invariant (ADR 0050)
• ➖ Requires new mapping propagation into telemetry and artifacts, increasing complexity

3. Minimal inline adoption logic in runAgent (no extracted helper)

• ➕ Fewer new exported concepts/types
• ➕ Less surface-area change in telemetry.New() signature
• ➖ Harder to unit test comprehensively (repeats fix(#2779): respect inbound TRACEPARENT in trace chain #2833 coverage gap)
• ➖ More error-prone and less readable as runAgent grows

Recommendation: Keep the PR’s approach: a small, explicit parser + a TraceContext value object makes the adoption rules testable and keeps propagation consistent (child env, recorder, summary). Using a full OTel propagator is plausible but likely overkill here, and decoupling the security id would undermine the project’s unified-correlation design.

Files changed (9) +475 / -69

Enhancement (2) +74 / -0

trace.goAdd IsShellSafeTraceID for adopted dashed-hex IDs +14/-0

Add IsShellSafeTraceID for adopted dashed-hex IDs

• Introduces a relaxed validator that enforces lowercase hex and UUID-shaped dashes without requiring UUIDv4 version/variant bits, enabling safe shell interpolation for adopted trace IDs.

internal/security/trace.go

trace.goW3C traceparent parsing, flag-preserving formatting, and UUID conversion +60/-0

W3C traceparent parsing, flag-preserving formatting, and UUID conversion

• Adds TraceParentWithFlags(), ParseTraceParent() (W3C forward-compatible parsing/validation), and UUIDFromTraceID() to convert a W3C trace-id into dashed UUID form for unified security correlation.

internal/telemetry/trace.go

Bug fix (2) +102 / -47

run.goAdopt inbound TRACEPARENT and propagate flags/remote parent +53/-17

Adopt inbound TRACEPARENT and propagate flags/remote parent

• Replaces unconditional trace-id generation with resolveTraceIdentity(), which adopts a valid inbound TRACEPARENT (trace-id, parent span-id, and flags) or falls back to fresh identity generation. Filters inherited TRACEPARENT from child-script environments and switches shell-safety validation to IsShellSafeTraceID for adopted (non-UUIDv4) IDs.

internal/cli/run.go

recorder.goRecorder now carries TraceContext (flags + remote parent) +49/-30

Recorder now carries TraceContext (flags + remote parent)

• Adds TraceContext and updates Recorder.New() to accept it, storing parentSpanID and flags. Ensures root span records a remote parent when continuing an inbound trace and that TraceParent()/run-summary.json preserve inbound trace-flags instead of hardcoding sampled.

internal/telemetry/recorder.go

Tests (5) +299 / -22

run_test.goTest accepting adopted (non-v4) trace IDs in scan context command +10/-0

Test accepting adopted (non-v4) trace IDs in scan context command

• Adds a unit test ensuring buildScanContextCommand does not replace adopted dashed-hex (non-UUIDv4) trace IDs with the invalid sentinel.

internal/cli/run_test.go

telemetry_run_test.goUnit tests for TRACEPARENT filtering and trace identity resolution +94/-0

Unit tests for TRACEPARENT filtering and trace identity resolution

• Adds tests that childScriptEnv removes pre-existing TRACEPARENT entries, still filters when telemetry is disabled, and preserves TRACESTATE. Adds table-style tests for resolveTraceIdentity() covering adoption, flag preservation, non-v4 adoption, and fallback behavior.

internal/cli/telemetry_run_test.go

trace_test.goTests for IsShellSafeTraceID acceptance/rejection cases +31/-0

Tests for IsShellSafeTraceID acceptance/rejection cases

• Adds coverage for generated UUIDv4 IDs, adopted non-v4 IDs, and multiple malformed or injection-like strings to ensure the validator enforces only the shell-safety property.

internal/security/trace_test.go

recorder_test.goUpdate tests for TraceContext and add remote-parent/flag assertions +89/-22

Update tests for TraceContext and add remote-parent/flag assertions

• Refactors recorder construction to use TraceContext and adds tests verifying the root span’s parent behavior (remote vs local root) and that summary/TraceParent preserve flags (including unsampled).

internal/telemetry/recorder_test.go

trace_test.goTests for ParseTraceParent, TraceParentWithFlags, and UUIDFromTraceID +75/-0

Tests for ParseTraceParent, TraceParentWithFlags, and UUIDFromTraceID

• Adds comprehensive test coverage for valid/invalid traceparent forms (including version rules, all-zero IDs, and uppercase rejection), flag formatting, and lossless round-trip conversion between dashed UUID and W3C trace-id.

internal/telemetry/trace_test.go

[github-actions]

Site preview

Preview: https://2efd4163-site.fullsend-ai.workers.dev

Commit: db1dff4dc2071b9ee854478fcac879c22d98fc86

[codecov]

Codecov Report

❌ Patch coverage is 91.66667% with 6 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/cli/run.go	73.91%	5 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 Respect a pre-existing TRACEPARENT in the security trace chain 🎫 feat: implement Level 2 distributed tracing — OTLP export to a backend

✅ Compliance rules (platform): 54 rules

✅ Skills: writing-user-docs, writing-adrs

1. RunnerEnv TRACEPARENT shadows ✓ Resolved 🐞 Bug ≡ Correctness

Description

childScriptEnv() filters TRACEPARENT only from os.Environ() but not from runnerEnv, so a
harness-provided runner_env TRACEPARENT can appear earlier than fullsend’s appended TRACEPARENT and
be the one observed by child scripts (first-match env lookup). This can also leak TRACEPARENT to
child scripts even when traceparent=="" (telemetry disabled).

Code

internal/cli/run.go[R1715-1726]

func childScriptEnv(runnerEnv map[string]string, traceparent string) []string {
-	env := append(os.Environ(), envToList(runnerEnv)...)
+	base := os.Environ()
+	env := make([]string, 0, len(base)+len(runnerEnv)+1)
+	for _, e := range base {
+		if !strings.HasPrefix(e, "TRACEPARENT=") {
+			env = append(env, e)
+		}
+	}
+	env = append(env, envToList(runnerEnv)...)
	if traceparent != "" {
		env = append(env, "TRACEPARENT="+traceparent)
	}

Relevance

⭐⭐⭐ High

Repo often accepts env precedence/shadowing fixes; reserved env-var shadowing hardening accepted in
PR #2582.

PR-#2582
PR-#2752

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The implementation only strips TRACEPARENT from the base environment, then appends runnerEnv entries
(which can include TRACEPARENT) before appending fullsend’s TRACEPARENT. Tests/comments in-repo
explicitly state env lookup is first-match, so any earlier runnerEnv TRACEPARENT will shadow
fullsend’s appended value.

internal/cli/run.go[1708-1727]
internal/cli/run.go[1745-1755]
internal/cli/run.go[598-604]
internal/harness/harness.go[241-260]
internal/cli/telemetry_run_test.go[124-140]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`childScriptEnv` removes inherited `TRACEPARENT` from the base process environment but then appends `runnerEnv` unchanged. If the harness specifies `runner_env.TRACEPARENT`, it will still be present *before* fullsend’s appended `TRACEPARENT`, and because env resolution is treated as **first match wins**, the child script will observe the harness value instead of fullsend’s. When `traceparent == ""`, a harness `runner_env.TRACEPARENT` also leaks through even though telemetry is intended to be disabled.

### Issue Context
This PR’s intent is to ensure exactly one effective `TRACEPARENT` is seen by child scripts and that it is fullsend’s propagation value.

### Fix Focus Areas
- internal/cli/run.go[1715-1727]
- internal/cli/telemetry_run_test.go[94-170]

### Suggested fix
1. In `childScriptEnv`, also filter out any `TRACEPARENT` entry coming from `runnerEnv` (e.g., skip key `TRACEPARENT` when appending runner env, or post-filter the `envToList(runnerEnv)` output using the same `strings.HasPrefix("TRACEPARENT=")` check).
2. Add/extend unit tests to cover:
  - `runnerEnv` includes `TRACEPARENT` and `traceparent != ""` ⇒ result contains exactly one `TRACEPARENT` and it equals fullsend’s.
  - `runnerEnv` includes `TRACEPARENT` and `traceparent == ""` ⇒ result contains zero `TRACEPARENT` entries.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 11:28 AM UTC · Completed 11:34 AM UTC
Commit: db1dff4 · View workflow run →

[fullsend-ai-retro]

PR #2960 (respect inbound TRACEPARENT in trace chain) had a well-functioning review workflow. The Qodo review agent found a real correctness bug — TRACEPARENT shadowing from runnerEnv in childScriptEnv() — which the author fixed within 4 minutes with new tests. The human reviewer (rh-hemartin) approved with no additional findings, meaning the agent fully covered review quality on this PR. One recurring gap persists: the PR touches internal/cli/ which AGENTS.md documents as requiring e2e tests, but e2e tests did not run (contributor needed ok-to-test label) and neither agent nor human flagged this. This exact gap is already tracked in issue #2036. No new proposals are warranted — the workflow was efficient (1 review iteration, ~17h to merge including overnight wait), and the single data point of agent success is not yet enough to justify autonomy changes.