diff --git a/internal/cli/run.go b/internal/cli/run.go index ca4f5a591c..96edf16607 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -586,13 +586,13 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep } } - // Trace identity (ADR 0050 Level 1 + security correlation). Generated here, + // Trace identity (ADR 0050 Level 1 + security correlation). Resolved here, // before the pre-script, so TRACEPARENT can propagate to child processes. - // The same id is reused as the security finding/audit trace id (dashed UUID) - // and, dash-stripped, as the W3C telemetry trace id — one id across both. - securityTraceID := security.GenerateTraceID() - wTraceID := telemetry.TraceIDFromUUID(securityTraceID) - rootSpanID := telemetry.NewSpanID() + // An inbound TRACEPARENT (nested/instrumented invocation, issue #2779) is + // adopted so the run continues the parent trace; otherwise a fresh id is + // generated. Either way one id serves as both the security finding/audit + // trace id (dashed UUID) and, dash-stripped, the W3C telemetry trace id. + securityTraceID, traceCtx := resolveTraceIdentity(os.Getenv("TRACEPARENT")) workItemID := resolveWorkItemID() // 2c. Run pre-script on the host (if configured). @@ -600,7 +600,7 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep preStart := time.Now() printer.StepStart("Running pre-script: " + h.PreScript) preCmd := exec.Command(h.PreScript) - preCmd.Env = childScriptEnv(h.RunnerEnv, telemetry.TraceParent(wTraceID, rootSpanID)) + preCmd.Env = childScriptEnv(h.RunnerEnv, telemetry.TraceParentWithFlags(traceCtx.TraceID, traceCtx.RootSpanID, traceCtx.Flags)) preCmd.Stdout = os.Stdout preCmd.Stderr = os.Stderr if err := preCmd.Run(); err != nil { @@ -626,7 +626,7 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // order — it runs last and the summary captures the whole run. var lastExitCode int var transcriptErrorOverride bool - rec := telemetry.New(runDir, wTraceID, rootSpanID, agentName, workItemID, runStart) + rec := telemetry.New(runDir, traceCtx, agentName, workItemID, runStart) defer func() { rec.Finalize(telemetryExitCode(lastExitCode, runErr)) }() createStart := time.Now() @@ -675,7 +675,7 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep printer.StepStart("Running post-script: " + h.PostScript) postCmd := exec.Command(h.PostScript) postCmd.Dir = runDir - postCmd.Env = childScriptEnv(h.RunnerEnv, telemetry.TraceParent(wTraceID, rootSpanID)) + postCmd.Env = childScriptEnv(h.RunnerEnv, telemetry.TraceParentWithFlags(traceCtx.TraceID, traceCtx.RootSpanID, traceCtx.Flags)) postCmd.Stdout = os.Stdout postCmd.Stderr = os.Stderr if err := postCmd.Run(); err != nil { @@ -1681,12 +1681,46 @@ func telemetryExitCode(lastExitCode int, runErr error) int { return lastExitCode } +// resolveTraceIdentity resolves the run's trace identity (issue #2779). A +// valid inbound W3C TRACEPARENT is adopted: its trace-id becomes both the +// security trace id (re-dashed UUID) and the W3C telemetry trace-id, its +// span-id becomes the root span's remote parent, and its trace-flags carry +// the upstream sampling decision forward. Without a valid inbound value a +// fresh identity is generated, exactly as Level 1 always did. TRACESTATE is +// intentionally untouched — it passes through os.Environ to child scripts. +func resolveTraceIdentity(inbound string) (securityTraceID string, tc telemetry.TraceContext) { + if traceID, parentSpanID, flags, ok := telemetry.ParseTraceParent(inbound); ok { + return telemetry.UUIDFromTraceID(traceID), telemetry.TraceContext{ + TraceID: traceID, + RootSpanID: telemetry.NewSpanID(), + ParentSpanID: parentSpanID, + Flags: flags, + } + } + securityTraceID = security.GenerateTraceID() + return securityTraceID, telemetry.TraceContext{ + TraceID: telemetry.TraceIDFromUUID(securityTraceID), + RootSpanID: telemetry.NewSpanID(), + Flags: "01", + } +} + // childScriptEnv builds the environment for a host-side child script (pre- or // post-script): the harness RunnerEnv layered over the process environment, -// plus the W3C TRACEPARENT for trace propagation (ADR 0050 Level 1). An empty -// traceparent (telemetry disabled) is omitted rather than emitted blank. +// plus the W3C TRACEPARENT for trace propagation (ADR 0050 Level 1). Any +// TRACEPARENT already present — inherited from the process environment or +// set in runner_env — is filtered out first: env lookups resolve the first +// match, so a stale value would shadow fullsend's own, and fullsend's trace +// identity never derives from runner_env (issue #2779). An empty traceparent +// (telemetry disabled) is omitted rather than emitted blank. func childScriptEnv(runnerEnv map[string]string, traceparent string) []string { - env := append(os.Environ(), envToList(runnerEnv)...) + merged := append(os.Environ(), envToList(runnerEnv)...) + env := make([]string, 0, len(merged)+1) + for _, e := range merged { + if !strings.HasPrefix(e, "TRACEPARENT=") { + env = append(env, e) + } + } if traceparent != "" { env = append(env, "TRACEPARENT="+traceparent) } @@ -1857,9 +1891,11 @@ func refreshOIDCToken(ctx context.Context, sandboxName, oidcURL, oidcAuth string // inside the sandbox. It finds known context files (including SKILL.md in // skill directories) in the repo directory and passes them as arguments. func buildScanContextCommand(repoDir, traceID string) string { - // Defense-in-depth: validate traceID before shell interpolation even though - // GenerateTraceID() only produces safe hex characters. - if !security.IsValidTraceID(traceID) { + // Defense-in-depth: validate traceID before shell interpolation. Uses + // IsShellSafeTraceID (not IsValidTraceID) because the id may have been + // adopted from an inbound W3C traceparent (issue #2779), so it is not + // necessarily UUID v4. + if !security.IsShellSafeTraceID(traceID) { // Should never happen with internal generation, but fail safely. traceID = "invalid-trace-id" } @@ -2196,10 +2232,10 @@ func scanOutputFiles(outputDir, traceID string, printer *ui.Printer) error { // injectTraceID appends the FULLSEND_TRACE_ID to the sandbox .env file. func injectTraceID(sandboxName, traceID string) error { - if !security.IsValidTraceID(traceID) { + if !security.IsShellSafeTraceID(traceID) { return fmt.Errorf("invalid trace ID format: %q", traceID) } - // Safe: IsValidTraceID() above ensures traceID matches UUID v4 format only. + // Safe: IsShellSafeTraceID() above ensures traceID is only hex and dashes. cmd := fmt.Sprintf("echo 'export FULLSEND_TRACE_ID=%s' >> %s/.env", traceID, sandbox.SandboxWorkspace) _, _, _, err := sandbox.Exec(sandboxName, cmd, 10*time.Second) return err diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index f3d9cf2292..cadf01d050 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -414,6 +414,16 @@ func TestBuildScanContextCommand_SourcesEnv(t *testing.T) { assert.Contains(t, cmd, "-exec fullsend scan context") } +func TestBuildScanContextCommand_AcceptsAdoptedTraceID(t *testing.T) { + // A trace id adopted from an inbound W3C traceparent (issue #2779) is + // dashed hex but not UUID v4; it must survive validation, not be replaced + // with the "invalid-trace-id" sentinel. + traceID := "4f3a9c1b-2d8e-0a7c-1f0b-1e2d3c4a5b6d" + cmd := buildScanContextCommand("/sandbox/workspace/repo", traceID) + assert.Contains(t, cmd, "FULLSEND_TRACE_ID='"+traceID+"'") + assert.NotContains(t, cmd, "invalid-trace-id") +} + func TestCopyFile(t *testing.T) { t.Run("copies content and preserves permissions", func(t *testing.T) { src := filepath.Join(t.TempDir(), "source") diff --git a/internal/cli/telemetry_run_test.go b/internal/cli/telemetry_run_test.go index 2563885259..1869e28b94 100644 --- a/internal/cli/telemetry_run_test.go +++ b/internal/cli/telemetry_run_test.go @@ -121,6 +121,137 @@ func TestChildScriptEnv_EmptyTraceparentOmitted(t *testing.T) { } } +func TestChildScriptEnv_FiltersPreExistingTraceparent(t *testing.T) { + // A parent process may already export TRACEPARENT (issue #2779). Most + // runtimes resolve the FIRST match in the environment, so the stale value + // must be filtered out — exactly one TRACEPARENT entry, fullsend's own. + t.Setenv("TRACEPARENT", "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1-bbbbbbbbbbbbbbbb-01") + const fullsendTP = "00-4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d-a1b2c3d4e5f60718-01" + + env := childScriptEnv(map[string]string{}, fullsendTP) + + traceparents := 0 + for _, e := range env { + if strings.HasPrefix(e, "TRACEPARENT=") { + traceparents++ + assert.Equal(t, "TRACEPARENT="+fullsendTP, e, "must be fullsend's value, not the parent's") + } + } + assert.Equal(t, 1, traceparents, "exactly one TRACEPARENT entry after filtering") +} + +func TestChildScriptEnv_EmptyTraceparentFiltersExisting(t *testing.T) { + // Even with telemetry disabled (empty traceparent), a stale inherited + // TRACEPARENT must not leak through to child scripts. + t.Setenv("TRACEPARENT", "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1-bbbbbbbbbbbbbbbb-01") + + env := childScriptEnv(map[string]string{}, "") + for _, e := range env { + assert.False(t, strings.HasPrefix(e, "TRACEPARENT="), "stale TRACEPARENT must be filtered even when disabled") + } +} + +func TestChildScriptEnv_FiltersRunnerEnvTraceparent(t *testing.T) { + // A harness-provided runner_env.TRACEPARENT would land before fullsend's + // appended value and win first-match resolution — same shadowing bug as + // the inherited process env, so it gets the same filter. fullsend's + // trace identity never derives from runner_env; honoring it would only + // desync child scripts from the recorded trace. + const fullsendTP = "00-4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d-a1b2c3d4e5f60718-01" + runnerEnv := map[string]string{ + "TRACEPARENT": "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1-bbbbbbbbbbbbbbbb-01", + "FOO": "bar", + } + + env := childScriptEnv(runnerEnv, fullsendTP) + + traceparents := 0 + hasFoo := false + for _, e := range env { + if strings.HasPrefix(e, "TRACEPARENT=") { + traceparents++ + assert.Equal(t, "TRACEPARENT="+fullsendTP, e, "must be fullsend's value, not runner_env's") + } + if e == "FOO=bar" { + hasFoo = true + } + } + assert.Equal(t, 1, traceparents, "exactly one TRACEPARENT entry") + assert.True(t, hasFoo, "other runner_env entries preserved") +} + +func TestChildScriptEnv_EmptyTraceparentFiltersRunnerEnv(t *testing.T) { + // Telemetry disabled: a runner_env TRACEPARENT must not leak either. + env := childScriptEnv(map[string]string{"TRACEPARENT": "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1-bbbbbbbbbbbbbbbb-01"}, "") + for _, e := range env { + assert.False(t, strings.HasPrefix(e, "TRACEPARENT="), "runner_env TRACEPARENT must be filtered when disabled") + } +} + +func TestChildScriptEnv_PreservesTracestate(t *testing.T) { + // W3C tracestate carries vendor context alongside traceparent and must + // pass through to child scripts untouched. + t.Setenv("TRACESTATE", "vendor=abc123,other=def456") + const tp = "00-4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d-a1b2c3d4e5f60718-01" + + env := childScriptEnv(map[string]string{}, tp) + + found := false + for _, e := range env { + if e == "TRACESTATE=vendor=abc123,other=def456" { + found = true + } + } + assert.True(t, found, "TRACESTATE must pass through to child scripts") +} + +func TestResolveTraceIdentity(t *testing.T) { + const ( + tid = "4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d" + sid = "a1b2c3d4e5f60718" + ) + reSpanID := regexp.MustCompile(`^[0-9a-f]{16}$`) + + t.Run("adopts valid inbound sampled traceparent", func(t *testing.T) { + securityID, tc := resolveTraceIdentity("00-" + tid + "-" + sid + "-01") + + assert.Equal(t, tid, tc.TraceID, "inbound trace-id adopted") + assert.Equal(t, sid, tc.ParentSpanID, "inbound span-id becomes the root span's remote parent") + assert.Equal(t, "01", tc.Flags) + assert.Equal(t, "4f3a9c1b-2d8e-4a7c-9f0b-1e2d3c4a5b6d", securityID, "security id derived from inbound trace-id") + assert.Equal(t, tid, telemetry.TraceIDFromUUID(securityID), "security id must round-trip to the W3C id") + assert.True(t, security.IsShellSafeTraceID(securityID)) + + assert.Regexp(t, reSpanID, tc.RootSpanID, "fresh root span id") + assert.NotEqual(t, sid, tc.RootSpanID, "root span is a child, not the inbound span itself") + }) + + t.Run("preserves unsampled flag", func(t *testing.T) { + _, tc := resolveTraceIdentity("00-" + tid + "-" + sid + "-00") + assert.Equal(t, "00", tc.Flags, "upstream unsampled decision preserved") + }) + + t.Run("adopted non-v4 trace-id is shell-safe", func(t *testing.T) { + // version nibble 0, variant nibble 1 — valid W3C, not UUID v4. + securityID, _ := resolveTraceIdentity("00-4f3a9c1b2d8e0a7c1f0b1e2d3c4a5b6d-" + sid + "-01") + assert.Equal(t, "4f3a9c1b-2d8e-0a7c-1f0b-1e2d3c4a5b6d", securityID) + assert.True(t, security.IsShellSafeTraceID(securityID)) + assert.False(t, security.IsValidTraceID(securityID), "adopted id is not v4 — needs the shell-safe validator") + }) + + for _, inbound := range []string{"", "not-a-traceparent", "00-" + tid + "-" + sid, "ff-" + tid + "-" + sid + "-01"} { + t.Run("falls back to fresh identity for "+fmt.Sprintf("%q", inbound), func(t *testing.T) { + securityID, tc := resolveTraceIdentity(inbound) + + assert.True(t, security.IsValidTraceID(securityID), "fresh id is a v4 UUID") + assert.Equal(t, telemetry.TraceIDFromUUID(securityID), tc.TraceID, "unified trace id (Level 1 invariant)") + assert.Empty(t, tc.ParentSpanID, "local trace root has no remote parent") + assert.Equal(t, "01", tc.Flags, "fresh traces are sampled") + assert.Regexp(t, reSpanID, tc.RootSpanID) + }) + } +} + func TestAgentSpanEndAttrs(t *testing.T) { var m agentruntime.RunMetrics m.Model = "claude-opus-4-6" diff --git a/internal/security/trace.go b/internal/security/trace.go index fb13e6852c..870ea49651 100644 --- a/internal/security/trace.go +++ b/internal/security/trace.go @@ -31,6 +31,20 @@ func IsValidTraceID(id string) bool { return reTraceID.MatchString(id) } +// reShellSafeTraceID matches any dashed lowercase-hex string in UUID shape +// (8-4-4-4-12), without the UUID v4 version/variant requirement of reTraceID. +var reShellSafeTraceID = regexp.MustCompile(`^[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}$`) + +// IsShellSafeTraceID returns true if the trace ID consists only of lowercase +// hex and dashes in UUID shape — the property that makes it safe for shell +// interpolation. Unlike IsValidTraceID it accepts any version/variant, because +// a trace id adopted from an inbound W3C traceparent (issue #2779) is not +// necessarily UUID v4. The charset restriction is the entire safety argument; +// version bits carry no interpolation risk. +func IsShellSafeTraceID(id string) bool { + return reShellSafeTraceID.MatchString(id) +} + // seedHash is the well-known genesis hash for the first entry in a chain. const seedHash = "0000000000000000000000000000000000000000000000000000000000000000" diff --git a/internal/security/trace_test.go b/internal/security/trace_test.go index 089cca9fa9..233053aabb 100644 --- a/internal/security/trace_test.go +++ b/internal/security/trace_test.go @@ -14,6 +14,37 @@ func TestGenerateTraceID(t *testing.T) { } } +func TestIsShellSafeTraceID(t *testing.T) { + // A generated UUID v4 passes both the strict and the shell-safe validator. + id := GenerateTraceID() + if !IsShellSafeTraceID(id) { + t.Errorf("generated trace ID %q should be shell-safe", id) + } + + // A trace id adopted from an inbound W3C traceparent is dashed hex but + // generally not UUID v4: shell-safe must accept it, strict must reject it. + adopted := "4f3a9c1b-2d8e-0a7c-1f0b-1e2d3c4a5b6d" // version 0, variant 0 + if !IsShellSafeTraceID(adopted) { + t.Error("adopted non-v4 trace ID should be shell-safe") + } + if IsValidTraceID(adopted) { + t.Error("adopted non-v4 trace ID should NOT pass strict v4 validation") + } + + for _, bad := range []string{ + "zzzzzzzz-zzzz-zzzz-zzzz-zzzzzzzzzzzz", // non-hex + "4F3A9C1B-2D8E-4A7C-9F0B-1E2D3C4A5B6D", // uppercase + "4f3a9c1b-2d8e", // wrong length + "4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d", // undashed + "", // empty + "4f3a9c1b-2d8e-4a7c-9f0b-1e2d3c4a5b6d; rm -rf /", // injection attempt + } { + if IsShellSafeTraceID(bad) { + t.Errorf("IsShellSafeTraceID(%q) must be false", bad) + } + } +} + func TestAppendFindingHashChain(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "findings.jsonl") diff --git a/internal/telemetry/recorder.go b/internal/telemetry/recorder.go index e2c4bb6fd9..34499b0c00 100644 --- a/internal/telemetry/recorder.go +++ b/internal/telemetry/recorder.go @@ -86,41 +86,60 @@ type spanState struct { start time.Time } +// TraceContext is the run's W3C trace identity. When the run continues an +// inbound TRACEPARENT (issue #2779), ParentSpanID carries the inbound span-id +// (so the root span joins the parent trace) and Flags carries the inbound +// trace-flags (so the upstream sampling decision is preserved). For a locally +// rooted trace both are empty; empty Flags means "01" (sampled). +type TraceContext struct { + TraceID string // 32-hex W3C trace-id + RootSpanID string // 16-hex span-id of this run's root span + ParentSpanID string // inbound remote parent span-id; "" = local trace root + Flags string // 2-hex W3C trace-flags; "" defaults to "01" +} + // Recorder writes crash-safe NDJSON telemetry and a final summary. The zero // value is not usable; obtain one from New. A nil *Recorder is a valid no-op. type Recorder struct { - mu sync.Mutex - f *os.File - dir string - traceID string - rootSpanID string - agent string - model string - workItem string - start time.Time - spans map[string]*spanState - steps []stepTiming - metrics *RunMetrics - disabled bool - finalized bool + mu sync.Mutex + f *os.File + dir string + traceID string + rootSpanID string + parentSpanID string + flags string + agent string + model string + workItem string + start time.Time + spans map[string]*spanState + steps []stepTiming + metrics *RunMetrics + disabled bool + finalized bool } // New opens /run-telemetry.jsonl for append and emits the root "run" span -// (backdated to start). traceID is a 32-hex W3C trace-id and rootSpanID a -// 16-hex span-id; both are supplied by the caller so the trace correlates with -// the run's security trace id and with child processes. +// (backdated to start). The trace identity is supplied by the caller so the +// trace correlates with the run's security trace id, with child processes, +// and — when continuing an inbound TRACEPARENT — with the parent trace. // // New never returns an error: if the file cannot be opened it returns a // disabled (no-op) recorder so the run is never affected. -func New(dir, traceID, rootSpanID, agent, workItemID string, start time.Time) *Recorder { +func New(dir string, tc TraceContext, agent, workItemID string, start time.Time) *Recorder { + if tc.Flags == "" { + tc.Flags = "01" + } r := &Recorder{ - dir: dir, - traceID: traceID, - rootSpanID: rootSpanID, - agent: agent, - workItem: workItemID, - start: start, - spans: make(map[string]*spanState), + dir: dir, + traceID: tc.TraceID, + rootSpanID: tc.RootSpanID, + parentSpanID: tc.ParentSpanID, + flags: tc.Flags, + agent: agent, + workItem: workItemID, + start: start, + spans: make(map[string]*spanState), } f, err := os.OpenFile(filepath.Join(dir, TelemetryFile), os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) if err != nil { @@ -131,8 +150,8 @@ func New(dir, traceID, rootSpanID, agent, workItemID string, start time.Time) *R r.mu.Lock() r.emit(eventRecord{ - V: SchemaVersion, Event: "span_start", TraceID: traceID, SpanID: rootSpanID, - Parent: "", Name: "run", TS: start.UTC().Format(time.RFC3339Nano), + V: SchemaVersion, Event: "span_start", TraceID: r.traceID, SpanID: r.rootSpanID, + Parent: r.parentSpanID, Name: "run", TS: start.UTC().Format(time.RFC3339Nano), WorkItemID: workItemID, Attrs: map[string]any{"agent": agent}, }) r.mu.Unlock() @@ -205,7 +224,7 @@ func (r *Recorder) TraceParent() string { if r.disabled { return "" } - return TraceParent(r.traceID, r.rootSpanID) + return TraceParentWithFlags(r.traceID, r.rootSpanID, r.flags) } // SetMetrics records aggregate run metrics to include in run-summary.json. @@ -264,12 +283,12 @@ func (r *Recorder) Finalize(exitCode int) { } r.emit(eventRecord{ V: SchemaVersion, Event: "span_end", TraceID: r.traceID, SpanID: r.rootSpanID, - Parent: "", Name: "run", TS: end.UTC().Format(time.RFC3339Nano), + Parent: r.parentSpanID, Name: "run", TS: end.UTC().Format(time.RFC3339Nano), WorkItemID: r.workItem, DurationMS: end.Sub(r.start).Milliseconds(), Status: status, }) r.writeSummary(runSummary{ - V: SchemaVersion, TraceID: r.traceID, Traceparent: TraceParent(r.traceID, r.rootSpanID), + V: SchemaVersion, TraceID: r.traceID, Traceparent: TraceParentWithFlags(r.traceID, r.rootSpanID, r.flags), Agent: r.agent, Model: r.model, WorkItemID: r.workItem, ExitCode: exitCode, StartedAt: r.start.UTC().Format(time.RFC3339Nano), EndedAt: end.UTC().Format(time.RFC3339Nano), DurationMS: end.Sub(r.start).Milliseconds(), Steps: r.steps, Metrics: r.metrics, diff --git a/internal/telemetry/recorder_test.go b/internal/telemetry/recorder_test.go index f77269c977..1af5775126 100644 --- a/internal/telemetry/recorder_test.go +++ b/internal/telemetry/recorder_test.go @@ -43,7 +43,7 @@ func readLines(t *testing.T, path string) []map[string]any { func TestRecorder_EmitsValidNDJSON(t *testing.T) { dir := t.TempDir() - r := New(dir, testTraceID, testRootID, "code", "octo/repo#2577", time.Now()) + r := New(dir, TraceContext{TraceID: testTraceID, RootSpanID: testRootID}, "code", "octo/repo#2577", time.Now()) sp := r.StartSpan("sandbox_create", "", nil) r.EndSpan(sp, "ok", nil) r.Finalize(0) @@ -62,7 +62,7 @@ func TestRecorder_EmitsValidNDJSON(t *testing.T) { func TestRecorder_RootSpanHasEmptyParentAndChildPointsToRoot(t *testing.T) { dir := t.TempDir() - r := New(dir, testTraceID, testRootID, "code", "wi", time.Now()) + r := New(dir, TraceContext{TraceID: testTraceID, RootSpanID: testRootID}, "code", "wi", time.Now()) sp := r.StartSpan("sandbox_create", "", nil) // empty parent => defaults to root r.EndSpan(sp, "ok", nil) r.Finalize(0) @@ -81,7 +81,7 @@ func TestRecorder_RootSpanHasEmptyParentAndChildPointsToRoot(t *testing.T) { func TestRecorder_SummaryFields(t *testing.T) { dir := t.TempDir() start := time.Now().Add(-2 * time.Second) - r := New(dir, testTraceID, testRootID, "code", "octo/repo#2577", start) + r := New(dir, TraceContext{TraceID: testTraceID, RootSpanID: testRootID}, "code", "octo/repo#2577", start) sp := r.StartSpan("agent", "", map[string]any{"iteration": 1}) r.EndSpan(sp, "ok", map[string]any{"iteration": 1, "exit_code": 0}) r.Finalize(0) @@ -123,7 +123,7 @@ func TestRecorder_SummaryFields(t *testing.T) { func TestRecorder_NonZeroExitMarksError(t *testing.T) { dir := t.TempDir() - r := New(dir, testTraceID, testRootID, "code", "wi", time.Now()) + r := New(dir, TraceContext{TraceID: testTraceID, RootSpanID: testRootID}, "code", "wi", time.Now()) r.Finalize(2) data, err := os.ReadFile(filepath.Join(dir, "run-summary.json")) @@ -145,7 +145,7 @@ func TestRecorder_NonZeroExitMarksError(t *testing.T) { func TestRecorder_CrashSafety_LinesDurableWithoutFinalize(t *testing.T) { dir := t.TempDir() - r := New(dir, testTraceID, testRootID, "code", "wi", time.Now()) + r := New(dir, TraceContext{TraceID: testTraceID, RootSpanID: testRootID}, "code", "wi", time.Now()) sp := r.StartSpan("sandbox_create", "", nil) r.EndSpan(sp, "ok", nil) // Simulate a crash: Finalize never called. Synced lines must still parse. @@ -179,7 +179,7 @@ func TestRecorder_TruncatedTrailingLineTolerated(t *testing.T) { func TestRecorder_GracefulDegradation_UnwritableDir(t *testing.T) { dir := filepath.Join(t.TempDir(), "does-not-exist") // parent missing => OpenFile fails - r := New(dir, testTraceID, testRootID, "code", "wi", time.Now()) + r := New(dir, TraceContext{TraceID: testTraceID, RootSpanID: testRootID}, "code", "wi", time.Now()) require.NotNil(t, r, "New must never return nil, even on failure") sp := r.StartSpan("x", "", nil) @@ -204,7 +204,7 @@ func TestRecorder_NilSafe(t *testing.T) { func TestRecorder_FinalizeIdempotentNoTmpLeft(t *testing.T) { dir := t.TempDir() - r := New(dir, testTraceID, testRootID, "code", "wi", time.Now()) + r := New(dir, TraceContext{TraceID: testTraceID, RootSpanID: testRootID}, "code", "wi", time.Now()) r.Finalize(0) assert.NotPanics(t, func() { r.Finalize(0) }, "second Finalize is a no-op") @@ -216,14 +216,14 @@ func TestRecorder_FinalizeIdempotentNoTmpLeft(t *testing.T) { func TestRecorder_TraceParent(t *testing.T) { dir := t.TempDir() - r := New(dir, testTraceID, testRootID, "code", "wi", time.Now()) + r := New(dir, TraceContext{TraceID: testTraceID, RootSpanID: testRootID}, "code", "wi", time.Now()) defer r.Finalize(0) assert.Equal(t, "00-"+testTraceID+"-"+testRootID+"-01", r.TraceParent()) } func TestRecorder_ConcurrentWritesNoCorruption(t *testing.T) { dir := t.TempDir() - r := New(dir, testTraceID, testRootID, "code", "wi", time.Now()) + r := New(dir, TraceContext{TraceID: testTraceID, RootSpanID: testRootID}, "code", "wi", time.Now()) var wg sync.WaitGroup for i := 0; i < 20; i++ { wg.Add(1) @@ -243,7 +243,7 @@ func TestRecorder_ConcurrentWritesNoCorruption(t *testing.T) { func TestRecorder_EndSpanUnknownSpanID(t *testing.T) { dir := t.TempDir() - r := New(dir, testTraceID, testRootID, "code", "wi", time.Now()) + r := New(dir, TraceContext{TraceID: testTraceID, RootSpanID: testRootID}, "code", "wi", time.Now()) r.EndSpan("deadbeefdeadbeef", "ok", nil) // never started — must not panic r.Finalize(0) @@ -261,7 +261,7 @@ func TestRecorder_EndSpanUnknownSpanID(t *testing.T) { func TestRecorder_WriteFailureMidRunDisablesGracefully(t *testing.T) { dir := t.TempDir() - r := New(dir, testTraceID, testRootID, "code", "wi", time.Now()) + r := New(dir, TraceContext{TraceID: testTraceID, RootSpanID: testRootID}, "code", "wi", time.Now()) // Simulate the telemetry file failing mid-run by closing it underneath. require.NoError(t, r.f.Close()) @@ -275,7 +275,7 @@ func TestRecorder_WriteFailureMidRunDisablesGracefully(t *testing.T) { func TestRecorder_SummaryWriteFailureSwallowed(t *testing.T) { dir := t.TempDir() - r := New(dir, testTraceID, testRootID, "code", "wi", time.Now()) + r := New(dir, TraceContext{TraceID: testTraceID, RootSpanID: testRootID}, "code", "wi", time.Now()) // Point the summary target at a non-existent subdir so WriteFile fails; // the failure must be swallowed and leave no stray temp file. r.dir = filepath.Join(dir, "missing") @@ -288,7 +288,7 @@ func TestRecorder_SummaryWriteFailureSwallowed(t *testing.T) { func TestRecorder_SummaryMetrics(t *testing.T) { dir := t.TempDir() - r := New(dir, testTraceID, testRootID, "code", "wi", time.Now()) + r := New(dir, TraceContext{TraceID: testTraceID, RootSpanID: testRootID}, "code", "wi", time.Now()) r.SetMetrics(RunMetrics{InputTokens: 18432, OutputTokens: 2901, CacheCreationInputTokens: 8000, CacheReadInputTokens: 50000, TotalCostUSD: 0.0731, NumTurns: 7, ToolCalls: 14}) r.Finalize(0) @@ -310,7 +310,7 @@ func TestRecorder_SummaryMetrics(t *testing.T) { func TestRecorder_SummaryModel(t *testing.T) { dir := t.TempDir() - r := New(dir, testTraceID, testRootID, "code", "wi", time.Now()) + r := New(dir, TraceContext{TraceID: testTraceID, RootSpanID: testRootID}, "code", "wi", time.Now()) r.SetModel("claude-opus-4-6") r.Finalize(0) @@ -323,7 +323,7 @@ func TestRecorder_SummaryModel(t *testing.T) { func TestRecorder_SummaryModelOmittedWhenUnset(t *testing.T) { dir := t.TempDir() - r := New(dir, testTraceID, testRootID, "code", "wi", time.Now()) + r := New(dir, TraceContext{TraceID: testTraceID, RootSpanID: testRootID}, "code", "wi", time.Now()) r.Finalize(0) // no SetModel data, err := os.ReadFile(filepath.Join(dir, SummaryFile)) require.NoError(t, err) @@ -336,13 +336,13 @@ func TestRecorder_SummaryModelOmittedWhenUnset(t *testing.T) { func TestRecorder_SetModelNilAndDisabledSafe(t *testing.T) { var r *Recorder assert.NotPanics(t, func() { r.SetModel("m") }) - disabled := New(filepath.Join(t.TempDir(), "does-not-exist"), testTraceID, testRootID, "code", "wi", time.Now()) + disabled := New(filepath.Join(t.TempDir(), "does-not-exist"), TraceContext{TraceID: testTraceID, RootSpanID: testRootID}, "code", "wi", time.Now()) assert.NotPanics(t, func() { disabled.SetModel("m") }) } func TestRecorder_FinalizeClosesFileEvenWhenDisabled(t *testing.T) { dir := t.TempDir() - r := New(dir, testTraceID, testRootID, "code", "wi", time.Now()) + r := New(dir, TraceContext{TraceID: testTraceID, RootSpanID: testRootID}, "code", "wi", time.Now()) r.disabled = true // simulate a mid-run emit failure that disabled the recorder r.Finalize(0) _, err := r.f.Write([]byte("x")) @@ -351,7 +351,7 @@ func TestRecorder_FinalizeClosesFileEvenWhenDisabled(t *testing.T) { func TestRecorder_EndSpanDefaultsEmptyStatusToOK(t *testing.T) { dir := t.TempDir() - r := New(dir, testTraceID, testRootID, "code", "wi", time.Now()) + r := New(dir, TraceContext{TraceID: testTraceID, RootSpanID: testRootID}, "code", "wi", time.Now()) sp := r.StartSpan("sandbox_create", "", nil) r.EndSpan(sp, "", nil) // empty status must default to "ok" r.Finalize(0) @@ -367,7 +367,7 @@ func TestRecorder_EndSpanDefaultsEmptyStatusToOK(t *testing.T) { func TestRecorder_EmitMarshalErrorDisablesRecorder(t *testing.T) { dir := t.TempDir() - r := New(dir, testTraceID, testRootID, "code", "wi", time.Now()) + r := New(dir, TraceContext{TraceID: testTraceID, RootSpanID: testRootID}, "code", "wi", time.Now()) // A channel cannot be JSON-marshaled, so emit hits its marshal-error branch // and must disable the recorder gracefully rather than panic. assert.NotPanics(t, func() { @@ -378,7 +378,7 @@ func TestRecorder_EmitMarshalErrorDisablesRecorder(t *testing.T) { func TestRecorder_SummaryRenameFailureSwallowed(t *testing.T) { dir := t.TempDir() - r := New(dir, testTraceID, testRootID, "code", "wi", time.Now()) + r := New(dir, TraceContext{TraceID: testTraceID, RootSpanID: testRootID}, "code", "wi", time.Now()) // Make the final summary path a non-empty directory so the atomic rename // fails; the failure must be swallowed and leave no stray temp file. blocker := filepath.Join(dir, SummaryFile) @@ -392,7 +392,7 @@ func TestRecorder_SummaryRenameFailureSwallowed(t *testing.T) { func TestRecorder_SummaryMetricsOmittedWhenUnset(t *testing.T) { dir := t.TempDir() - r := New(dir, testTraceID, testRootID, "code", "wi", time.Now()) + r := New(dir, TraceContext{TraceID: testTraceID, RootSpanID: testRootID}, "code", "wi", time.Now()) r.Finalize(0) // no SetMetrics data, err := os.ReadFile(filepath.Join(dir, SummaryFile)) @@ -407,6 +407,73 @@ func TestRecorder_SetMetricsNilAndDisabledSafe(t *testing.T) { var r *Recorder assert.NotPanics(t, func() { r.SetMetrics(RunMetrics{InputTokens: 1}) }) - disabled := New(filepath.Join(t.TempDir(), "does-not-exist"), testTraceID, testRootID, "code", "wi", time.Now()) + disabled := New(filepath.Join(t.TempDir(), "does-not-exist"), TraceContext{TraceID: testTraceID, RootSpanID: testRootID}, "code", "wi", time.Now()) assert.NotPanics(t, func() { disabled.SetMetrics(RunMetrics{InputTokens: 1}) }) } + +func TestRecorder_RootSpanRecordsRemoteParent(t *testing.T) { + // When the run adopts an inbound TRACEPARENT (issue #2779), the root span + // must record the inbound span-id as its parent so the exported trace + // joins the parent chain instead of orphaning itself. + dir := t.TempDir() + r := New(dir, TraceContext{ + TraceID: testTraceID, RootSpanID: testRootID, + ParentSpanID: "beefbeefbeefbeef", Flags: "01", + }, "code", "wi", time.Now()) + r.Finalize(0) + + lines := readLines(t, filepath.Join(dir, TelemetryFile)) + require.Len(t, lines, 2) // root span_start + span_end + for _, m := range lines { + assert.Equal(t, "run", m["name"]) + assert.Equal(t, "beefbeefbeefbeef", m["parent"], "root span must carry the inbound remote parent") + } +} + +func TestRecorder_RootSpanEmptyParentWhenLocalRoot(t *testing.T) { + // No inbound traceparent: the root span has no parent, as in Level 1. + dir := t.TempDir() + r := New(dir, TraceContext{TraceID: testTraceID, RootSpanID: testRootID}, + "code", "wi", time.Now()) + r.Finalize(0) + + lines := readLines(t, filepath.Join(dir, TelemetryFile)) + require.Len(t, lines, 2) + assert.Equal(t, "", lines[0]["parent"], "local trace root has empty parent") +} + +func TestRecorder_SummaryTraceparentPreservesFlags(t *testing.T) { + // An upstream-unsampled trace (flags 00) must not be re-advertised as + // sampled by the summary or by TraceParent() — a downstream consumer + // chaining from either would resurrect a dead sampling decision. + dir := t.TempDir() + r := New(dir, TraceContext{ + TraceID: testTraceID, RootSpanID: testRootID, Flags: "00", + }, "code", "wi", time.Now()) + + wantTP := "00-" + testTraceID + "-" + testRootID + "-00" + assert.Equal(t, wantTP, r.TraceParent(), "TraceParent() must carry the inbound flags") + + r.Finalize(0) + data, err := os.ReadFile(filepath.Join(dir, SummaryFile)) + require.NoError(t, err) + var s map[string]any + require.NoError(t, json.Unmarshal(data, &s)) + assert.Equal(t, wantTP, s["traceparent"], "summary traceparent must carry the inbound flags") +} + +func TestRecorder_EmptyFlagsDefaultTo01(t *testing.T) { + dir := t.TempDir() + r := New(dir, TraceContext{TraceID: testTraceID, RootSpanID: testRootID}, + "code", "wi", time.Now()) + + want := TraceParent(testTraceID, testRootID) + assert.Equal(t, want, r.TraceParent(), "empty flags must behave exactly like Level 1 (sampled)") + + r.Finalize(0) + data, err := os.ReadFile(filepath.Join(dir, SummaryFile)) + require.NoError(t, err) + var s map[string]any + require.NoError(t, json.Unmarshal(data, &s)) + assert.Equal(t, want, s["traceparent"]) +} diff --git a/internal/telemetry/trace.go b/internal/telemetry/trace.go index 6061caaa75..425fa8d5d5 100644 --- a/internal/telemetry/trace.go +++ b/internal/telemetry/trace.go @@ -33,6 +33,66 @@ func TraceParent(traceID, spanID string) string { return "00-" + traceID + "-" + spanID + "-01" } +// TraceParentWithFlags is like TraceParent but carries the given trace-flags +// (2 lowercase hex chars) so an upstream sampling decision is preserved when +// continuing an inbound trace. Empty flags default to "01" (sampled). +func TraceParentWithFlags(traceID, spanID, flags string) string { + if flags == "" { + flags = "01" + } + return "00-" + traceID + "-" + spanID + "-" + flags +} + +// ParseTraceParent parses a W3C traceparent header value and returns its +// trace-id, parent span-id, and trace-flags. ok is false when the value is +// malformed. Per the W3C spec: version "ff" is forbidden; version "00" has +// exactly four fields; higher versions are parsed forward-compatibly (the +// first four fields, ignoring any additions). +func ParseTraceParent(tp string) (traceID, spanID, flags string, ok bool) { + parts := strings.Split(tp, "-") + if len(parts) < 4 { + return "", "", "", false + } + version := parts[0] + if len(version) != 2 || !isLowerHex(version) || version == "ff" { + return "", "", "", false + } + if version == "00" && len(parts) != 4 { + return "", "", "", false + } + traceID, spanID, flags = parts[1], parts[2], parts[3] + if len(traceID) != 32 || !isLowerHex(traceID) || traceID == "00000000000000000000000000000000" { + return "", "", "", false + } + if len(spanID) != 16 || !isLowerHex(spanID) || spanID == "0000000000000000" { + return "", "", "", false + } + if len(flags) != 2 || !isLowerHex(flags) { + return "", "", "", false + } + return traceID, spanID, flags, true +} + +// UUIDFromTraceID converts a 32-hex W3C trace-id into dashed UUID form +// (8-4-4-4-12) so an adopted inbound trace-id can serve as the run's security +// trace id. Returns "" unless the input is exactly 32 lowercase hex chars. +func UUIDFromTraceID(traceID string) string { + if len(traceID) != 32 || !isLowerHex(traceID) { + return "" + } + return traceID[0:8] + "-" + traceID[8:12] + "-" + traceID[12:16] + "-" + traceID[16:20] + "-" + traceID[20:32] +} + +// isLowerHex reports whether s consists only of lowercase hex characters. +func isLowerHex(s string) bool { + for _, c := range s { + if (c < '0' || c > '9') && (c < 'a' || c > 'f') { + return false + } + } + return true +} + // randRead is a seam over crypto/rand.Read so the RNG-failure fallback in // randomHex is testable. var randRead = rand.Read diff --git a/internal/telemetry/trace_test.go b/internal/telemetry/trace_test.go index c5fb6f75af..71ce8e3575 100644 --- a/internal/telemetry/trace_test.go +++ b/internal/telemetry/trace_test.go @@ -84,3 +84,78 @@ func TestTraceParent(t *testing.T) { require.Equal(t, "00-4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d-a1b2c3d4e5f60718-01", got) assert.Regexp(t, reTraceparent, got) } + +func TestTraceParentWithFlags(t *testing.T) { + got := TraceParentWithFlags("4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d", "a1b2c3d4e5f60718", "00") + assert.Equal(t, "00-4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d-a1b2c3d4e5f60718-00", got, "unsampled flag preserved") + + got = TraceParentWithFlags("4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d", "a1b2c3d4e5f60718", "01") + assert.Equal(t, TraceParent("4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d", "a1b2c3d4e5f60718"), got, + "sampled flags must match the TraceParent default") + + got = TraceParentWithFlags("4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d", "a1b2c3d4e5f60718", "") + assert.Equal(t, "00-4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d-a1b2c3d4e5f60718-01", got, + "empty flags default to sampled") +} + +func TestParseTraceParent(t *testing.T) { + const ( + tid = "4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d" + sid = "a1b2c3d4e5f60718" + ) + tests := []struct { + name string + input string + wantTID string + wantSID string + wantF string + wantOK bool + }{ + {name: "valid sampled", input: "00-" + tid + "-" + sid + "-01", wantTID: tid, wantSID: sid, wantF: "01", wantOK: true}, + {name: "valid unsampled", input: "00-" + tid + "-" + sid + "-00", wantTID: tid, wantSID: sid, wantF: "00", wantOK: true}, + // W3C forward compatibility: a higher version with the version-00 + // fields intact must parse (future versions may append fields). + {name: "future version exact fields", input: "cc-" + tid + "-" + sid + "-01", wantTID: tid, wantSID: sid, wantF: "01", wantOK: true}, + {name: "future version extra field", input: "cc-" + tid + "-" + sid + "-01-extradata", wantTID: tid, wantSID: sid, wantF: "01", wantOK: true}, + // W3C: version 00 has exactly four fields; trailing data is invalid. + {name: "version 00 with extra field", input: "00-" + tid + "-" + sid + "-01-extradata"}, + // W3C: version ff is forbidden. + {name: "version ff", input: "ff-" + tid + "-" + sid + "-01"}, + {name: "non-hex version", input: "zz-" + tid + "-" + sid + "-01"}, + {name: "empty", input: ""}, + {name: "too few parts", input: "00-" + tid + "-" + sid}, + {name: "all-zero trace-id", input: "00-00000000000000000000000000000000-" + sid + "-01"}, + {name: "all-zero span-id", input: "00-" + tid + "-0000000000000000-01"}, + {name: "uppercase hex", input: "00-4F3A9C1B2D8E4A7C9F0B1E2D3C4A5B6D-" + sid + "-01"}, + {name: "short trace-id", input: "00-4f3a9c1b2d8e4a7c-" + sid + "-01"}, + {name: "short span-id", input: "00-" + tid + "-a1b2c3d4-01"}, + {name: "short flags", input: "00-" + tid + "-" + sid + "-1"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + gotTID, gotSID, gotF, ok := ParseTraceParent(tc.input) + assert.Equal(t, tc.wantOK, ok, "ok mismatch") + if tc.wantOK { + assert.Equal(t, tc.wantTID, gotTID) + assert.Equal(t, tc.wantSID, gotSID) + assert.Equal(t, tc.wantF, gotF) + } else { + assert.Empty(t, gotTID) + assert.Empty(t, gotSID) + assert.Empty(t, gotF) + } + }) + } +} + +func TestUUIDFromTraceID(t *testing.T) { + got := UUIDFromTraceID("4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d") + assert.Equal(t, "4f3a9c1b-2d8e-4a7c-9f0b-1e2d3c4a5b6d", got) + + // Round-trip with TraceIDFromUUID must be lossless. + assert.Equal(t, "4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d", TraceIDFromUUID(got)) + + assert.Empty(t, UUIDFromTraceID("tooshort")) + assert.Empty(t, UUIDFromTraceID("4F3A9C1B2D8E4A7C9F0B1E2D3C4A5B6D"), "uppercase is not valid W3C") + assert.Empty(t, UUIDFromTraceID("4f3a9c1b2d8e4a7c9f0b1e2d3c4a5bzz"), "non-hex rejected") +}