refactor(telemetry)!: replace bespoke recorder with OTel Go SDK

[rh-hemartin]

Summary

Replaces the two-layer telemetry system with the OpenTelemetry Go SDK (#2780).

Before: A hand-rolled NDJSON recorder wrote run-telemetry.jsonl and run-summary.json at runtime. After the run finished, a separate package read those files back and reconstructed OTel spans for OTLP export.

After: The OTel SDK records spans directly through configured exporters. A file exporter writes OTLP JSON to run-telemetry.jsonl; an OTLP HTTP exporter sends to a remote backend when OTEL_EXPORTER_OTLP_ENDPOINT is set. No post-run replay step.

Breaking change

The bare agent attribute on the run span is now fullsend.agent. Consumers querying by the old key must update their queries.

What changed

• Security trace ID decoupled from telemetry: generated independently via crypto/rand instead of derived from the SDK's trace ID.
• W3C TRACEPARENT propagation uses the SDK's built-in propagator instead of hand-rolled parsing.
• Root span carries all run metadata (exit code, model, tokens, cost, turns, tool calls, iterations), eliminating run-summary.json as a separate artifact.
• parentSampledProcessor suppresses the entire trace from OTLP export when the remote parent is unsampled, not just the root span.
• New hack/upload-traces.sh replays trace files into any OTLP backend via otelcol-contrib, replacing the deleted Go replay tool.
• Noop tracer on setup failure; telemetry never affects the run.

ADR: docs/ADRs/0050-distributed-tracing-instrumentation.md.

Test plan

• go vet and go test pass for affected packages
• Local run with OTLP endpoint: spans received by backend with correct parent-child hierarchy
• Local run without OTLP endpoint: file written, no network calls, run unaffected
• CI green

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 92.69103% with 22 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/telemetry/fileexporter.go	93.24%	9 Missing and 6 partials ⚠️
internal/telemetry/telemetry.go	90.54%	6 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:05 AM UTC · Completed 8:18 AM UTC
Commit: 36e2ef8 · View workflow run →

[fullsend-ai-review]

Review

Re-review (prior: d9c4077, provenance: app-verified). All 22 PR files changed since prior review; all findings re-evaluated independently. The prior HIGH logic-error finding (security trace ID decoupled) is resolved — fullsend.security_trace_id is now recorded as a span attribute on the root span. The prior MEDIUM scope-alignment finding (unrelated documentation files) is resolved — those files are no longer in the diff. The prior MEDIUM behavioral-change finding (OTEL_SDK_DISABLED doc mismatch) remains resolved.

Findings

High

• [breaking-artifact] internal/telemetry/telemetry.go — run-summary.json artifact removed entirely. External scripts, CI workflows, or tooling parsing run-summary.json to extract run metadata (exit code, agent name, model, traceparent, token counts, cost) will break. Consumers must migrate to parsing run-telemetry.jsonl (OTLP JSON format) or querying the OTLP backend directly.
  Remediation: Verify all known consumers within the fullsend ecosystem have been migrated. Provide example jq commands to extract common fields from the new OTLP JSON format.

• [breaking-artifact] internal/telemetry/fileexporter.go — run-telemetry.jsonl format changed from bespoke NDJSON (v1 schema with span_start/span_end events, RFC3339 timestamps, JSON numbers) to OTLP JSON (one TracesData message per line with hex-encoded trace/span IDs, nanosecond timestamps, typed attribute values). External consumers parsing the old format will fail.
  Remediation: Ensure migration guidance reaches external adopters. Standard OTLP tooling (otelcol-contrib receivers, OTLP parsers) can now consume the file directly.

Medium

• [breaking-attribute] internal/cli/run.go — Attribute rename from bare agent to fullsend.agent on the run span. Consumers with queries or dashboards filtering on the old attribute key must update. Documented breaking change (! suffix in PR title).
  Remediation: Document the rename in release notes.

• [breaking-go-api] internal/telemetry/telemetry.go — Multiple exported Go symbols removed from package telemetry: Recorder, TraceContext, RunMetrics, New, TraceParent, ParseTraceParent, etc. Entire otlp sub-package deleted. The internal/ path limits direct external imports but forked repos or vendored copies will break.

• [naming-convention] internal/telemetry/telemetry.go — Setup writes OTLP validation warnings to os.Stderr with a novel fullsend: prefix. The existing codebase uses printer.StepWarn() or WARNING: prefix for operational diagnostics.
  Remediation: Consider accepting an optional warn callback or using the established WARNING: prefix.

Low

• [logic-error] internal/telemetry/telemetry.go — parentSampledProcessor.OnStart calls p.base.OnStart(parent, s) unconditionally for suppressed spans while OnEnd suppresses them. BatchSpanProcessor.OnStart is a no-op, so no practical impact.

• [test-integrity] internal/telemetry/telemetry_test.go — TestSetup_FileExporter calls cleanup(context.Background()) both explicitly and via defer, causing double shutdown/close on already-closed resources.

• [behavioral-change] internal/cli/run.go — Root span start time now reflects the moment tracer.Start is called (after run directory creation) rather than the start of runAgent.

• [scope-creep] internal/cli/run.go — Security trace ID decoupled from W3C trace ID. Now an independent UUID (security.GenerateTraceID()) recorded as fullsend.security_trace_id span attribute. Natural consequence of OTel SDK migration.

• [data-exposure] internal/cli/run.go — runErr.Error() recorded in span status messages. Standard OTel practice; metadata-class data.

• [data-exposure] internal/telemetry/telemetry.go — OTEL_SDK_DISABLED=true suppresses ALL telemetry including the local file. Documented in the updated guide. Use OTEL_TRACES_EXPORTER=none to suppress only OTLP export.

• [adr-immutability-violation] docs/ADRs/0050-distributed-tracing-instrumentation.md — ADR 0050 annotation section added. Falls within AGENTS.md allowance for short notes.

• [breaking-attribute] internal/cli/run.go — fullsend.work_item_id scope narrowed from every span to root run span only. Docs updated accordingly.

• [naming-convention] internal/telemetry/trace.go — TraceParent → Traceparent (treating traceparent as one word per W3C spec).

• [step-numbering] internal/cli/run.go — Mixed step numbering schemes (new 4/4a/5 vs surviving 9e-bis).

• [comment-removal] internal/cli/telemetry_run_test.go — Multiple test documentation comments removed that explained non-obvious invariants (TRACEPARENT shadowing, stale env leakage).

Previous run

Review

Re-review (prior: 3dc2ca4, provenance: app-verified). All 24 PR files changed since prior review; all findings re-evaluated independently. The prior HIGH logic-error finding (security trace ID decoupled) is resolved — fullsend.security_trace_id is now recorded as a span attribute on the root span, maintaining cross-domain joinability. The prior MEDIUM behavioral-change finding (OTEL_SDK_DISABLED doc mismatch) is resolved — the distributed-tracing guide now correctly documents the behavior.

Findings

High

• [breaking-api] internal/cli/run.go — run-summary.json artifact removed entirely. This was a documented Level 1 artifact (ADR 0050) that external scripts, CI workflows, or tooling may parse to extract run metadata (exit code, agent name, model, traceparent, token counts, cost). Consumers must migrate to parsing run-telemetry.jsonl (OTLP JSON format) or query the OTLP backend directly.
  Remediation: Document the migration path in release notes. Provide example jq commands to extract common fields from the new OTLP JSON format as a replacement for run-summary.json queries.

• [breaking-api] internal/telemetry/fileexporter.go — run-telemetry.jsonl format changed from bespoke NDJSON (v1 schema with span_start/span_end events, RFC3339 timestamps, JSON numbers) to OTLP JSON (one TracesData message per line with hex-encoded trace/span IDs, nanosecond timestamps, typed attribute values). External consumers parsing the old format will fail to parse the new structure.
  Remediation: Document the new OTLP JSON schema in the tracing guide. Note that standard OTLP tooling (otelcol-contrib receivers, OTLP parsers) can now consume the file directly.

Medium

• [scope-alignment] docs/guides/dev/testing-agent-changes.md, docs/guides/user/rate-limiting.md — Two new documentation files (167 lines total) are unrelated to the telemetry refactor authorized by issue Explore the OpenTelemetry Go SDK for Level 2 trace export #2780. testing-agent-changes.md explains how to test agent policy/harness changes by overriding SHA refs; rate-limiting.md describes rate limiting configuration. Neither references telemetry, tracing, or OTel. These belong in separate PRs.

• [breaking-api] internal/cli/run.go — Attribute rename from bare agent to fullsend.agent on the run span. Consumers with queries or dashboards filtering on the old attribute key (e.g., attributes.agent = 'code') must update to fullsend.agent. Documented breaking change (! suffix in PR title).

Low

• [logic-error] internal/telemetry/telemetry.go — parentSampledProcessor.OnStart calls p.base.OnStart(parent, s) unconditionally — even for suppressed spans — while OnEnd suppresses them. The base processor (BatchSpanProcessor) has a no-op OnStart, so there is no practical impact in this codebase, but the asymmetry is a code hygiene issue.

• [adr-immutability-violation] docs/ADRs/0050-distributed-tracing-instrumentation.md — ADR 0050 has status Accepted on main. An annotation section was added documenting the run-summary.json removal and OTLP export model change. The annotation is well-formed (short, cites commit SHA, describes what changed) and falls within the AGENTS.md allowance for "short notes linking to newer decisions." However, whether the Level 1 artifact specification change warrants a superseding ADR is a judgment call for the maintainers.

• [breaking-api] internal/cli/run.go — fullsend.work_item_id attribute scope narrowed from every span to only the root run span. The docs are updated accordingly. In standard OTel practice, parent attributes are queryable via trace-aware backends through parent joins. Practical impact is low.

• [edge-case] internal/telemetry/fileexporter.go — convertValue for FLOAT64 emits v.AsFloat64() directly. If the value is NaN or ±Inf, json.Marshal returns an error and the span batch is dropped. Extremely unlikely in practice (no standard OTel API produces NaN/Inf attributes).

• [behavioral-change] internal/cli/run.go — The root span's start time now reflects the moment tracer.Start is called (after run directory creation), rather than the start of the entire runAgent invocation. The root span no longer covers validation and provider resolution. Practical impact is small since those steps are typically sub-second.

• [data-exposure] internal/cli/run.go — runErr.Error() is recorded in OTel span status messages via rootSpan.SetStatus(codes.Error, ...), agentSpan.SetStatus(...), and sandboxSpan.SetStatus(...). The prior implementation recorded only fixed "ok"/"error" strings. Error messages may contain internal paths or API details. Standard OTel practice; metadata-class data.

• [data-exposure] internal/telemetry/telemetry.go — OTEL_SDK_DISABLED=true now suppresses ALL telemetry output including the local run-telemetry.jsonl forensic file. Previously, local files were always written. The distributed-tracing guide is updated to document this. An operator setting this env var eliminates the crash-forensic safety net.

• [test-integrity] internal/telemetry/telemetry_test.go — TestSetup_FileExporter calls cleanup(context.Background()) both explicitly and via defer, causing double shutdown/close on already-closed resources. While the OTel SDK's Shutdown is idempotent, this is sloppy test code that could mask future issues.

Previous run (2)

Review

Re-review (prior: 889c0b4, provenance: app-verified). All PR files changed since prior review; all findings re-evaluated independently. The sampling architecture changed from the prior review's ParentBased(AlwaysSample()) to AlwaysSample() + a custom parentSampledProcessor that suppresses entire unsampled traces from OTLP via a sync.Map keyed by trace ID. The prior MEDIUM parentSampledProcessor finding is resolved — OnStart now stores unsampled trace IDs and OnEnd checks the trace ID rather than just the parent span context. The fileExporter now includes f.Sync() per span (crash-safety concern resolved). The buildResourceSpans value-copy issue from earlier reviews is resolved — the new implementation rebuilds output from maps after all spans are collected.

Findings

High

• [logic-error] internal/cli/run.go — The security trace ID (securityTraceID) and the OTel trace ID are fully decoupled. securityTraceID is generated independently via security.GenerateTraceID() while the OTel trace ID is auto-generated by the SDK's TracerProvider. This breaks the unified-trace-identity invariant that the deleted TestTraceIDUnification test explicitly guarded: a single ID correlated security findings (FULLSEND_TRACE_ID in the sandbox), telemetry spans, and child traces. No replacement correlation mechanism is provided — securityTraceID is not recorded as a span attribute, and the OTel trace ID is not exposed to security systems. The PR body acknowledges this as intentional but provides no mechanism for forensic cross-correlation between security findings and telemetry traces.
  Remediation: Record securityTraceID as a span attribute (attribute.String("fullsend.security_trace_id", securityTraceID)) on the root span so the two domains remain joinable. This preserves the decoupled design while maintaining observability.

Medium

• [behavioral-change] internal/telemetry/telemetry.go:54 — OTEL_SDK_DISABLED=true now suppresses Level 1 local file output entirely. The isSDKDisabled() check at line 54 returns a noop tracer and skips file creation. Previously, OTEL_SDK_DISABLED only affected the OTLP export path (in otlp.ExportRunDir); the bespoke recorder always wrote run-telemetry.jsonl. The distributed-tracing guide (updated in this PR) states "This file is always written, even when no OTLP backend is configured" — but with OTEL_SDK_DISABLED=true, it is not written.
  Remediation: Move the isSDKDisabled() check to guard only the OTLP exporter creation (not the file exporter), or update docs/guides/infrastructure/distributed-tracing.md to note that OTEL_SDK_DISABLED=true suppresses all telemetry output including the local file.

• [adr-immutability-violation] docs/ADRs/0050-distributed-tracing-instrumentation.md — ADR 0050 has status Accepted on main. This PR modifies its Decision section: removing run-summary.json from the Level 1 artifact description and updating a Context section cross-reference. Per AGENTS.md, accepted ADRs are point-in-time records — substantive changes require a new superseding ADR. The core decision (three-level opt-in, OTel-native, W3C propagation) is unchanged, but the Level 1 artifact specification is modified in place.
  Remediation: Either (a) create a superseding ADR documenting the run-summary.json removal and the live OTLP export change, updating ADR 0050's status to Superseded, or (b) limit changes to a short annotation noting that run-summary.json was superseded by root-span attributes per this refactor.

Low

• [edge-case] internal/telemetry/fileexporter.go:146 — convertValue for FLOAT64 emits v.AsFloat64() directly. If the value is NaN or ±Inf, json.Marshal returns an error. Unlike write errors (which set e.failed = true), marshal errors are returned without disabling the exporter. Extremely unlikely in practice (no standard OTel API produces NaN/Inf attributes).

• [scope-alignment] internal/telemetry — Issue Explore the OpenTelemetry Go SDK for Level 2 trace export #2780 describes exploratory work ("Explore the OpenTelemetry Go SDK"). The PR implements a committed breaking change (removing run-summary.json, renaming the agent attribute to fullsend.agent). Verify the breaking change scope has been authorized beyond the exploratory issue.

• [error-handling] internal/cli/run.go — tracingCleanup(flushCtx) in the defer block has no recover(). The old Recorder.emit() had an explicit defer/recover to ensure panics in the telemetry path never crashed the run. The OTel SDK is production-grade and unlikely to panic during shutdown, but wrapping in recover() would maintain the fail-open contract.

• [behavioral-change] internal/cli/run.go — The run directory is now created before the pre-script (step 3 → step 4), whereas previously the pre-script ran before directory creation. Risk is negligible since the run directory path (PID + timestamp) is unpredictable and not communicated to pre-scripts.

• [data-exposure] internal/cli/run.go — runErr.Error() is recorded in span status messages via rootSpan.SetStatus(codes.Error, runErr.Error()), agentSpan.SetStatus(codes.Error, ...), and sandboxSpan.SetStatus(codes.Error, ...). The old recorder only recorded fixed "ok"/"error" strings. Error messages may contain internal paths or API details. This is standard OTel practice and the data is metadata-class, but it is a net increase in information exported.

• [test-adequacy] internal/telemetry/telemetry_test.go — No end-to-end test exercises the unsampled inbound TRACEPARENT scenario through the W3C propagator + parentSampledProcessor pipeline. TestParentSampledProcessor_SuppressesEntireTrace validates the processor in isolation but does not cover the full Setup path.

• [test-adequacy] internal/telemetry/fileexporter_test.go — No concurrency tests for the fileExporter. The SimpleSpanProcessor serializes calls, but the mutex should still be tested under concurrent access to guard against future processor changes.

• [breaking-api] internal/cli/run.go — Attribute rename from bare agent to fullsend.agent on the run span. Documented breaking change (PR title ! suffix). Consumers querying by the old key must update.

• [breaking-api] internal/telemetry/recorder.go — run-summary.json artifact removed and telemetry file format changed from bespoke NDJSON to OTLP JSON. Documented breaking change.

Previous run (3)

Review

Re-review (prior: 594cc9c, provenance: app-verified). The sampling architecture changed from the prior review's ParentBased(AlwaysSample()) to AlwaysSample() + a custom parentSampledProcessor that suppresses entire unsampled traces from OTLP via a sync.Map keyed by trace ID. The prior MEDIUM parentSampledProcessor finding is resolved — OnStart now stores unsampled trace IDs and OnEnd checks the trace ID rather than just the parent span context. The fileExporter now includes f.Sync() per span (crash-safety concern resolved). A new TestParentSampledProcessor_SuppressesEntireTrace test validates the fix.

Findings

High

• [logic-error] internal/cli/run.go:822 — The security trace ID (securityTraceID) and the OTel trace ID are now fully decoupled. securityTraceID is generated independently via security.GenerateTraceID() while the OTel trace ID is auto-generated by the SDK's TracerProvider. This breaks the unified-trace-identity invariant that the deleted TestTraceIDUnification test explicitly guarded: a single ID correlated security findings (FULLSEND_TRACE_ID in the sandbox), telemetry spans, and child traces. No replacement correlation mechanism is provided — securityTraceID is not recorded as a span attribute, and the OTel trace ID is not exposed to security systems. The PR body acknowledges this as intentional but does not document how forensic cross-correlation between security findings and telemetry traces should work without the unified ID.
  Remediation: Either (a) derive securityTraceID from the OTel root span's trace ID after tracer.Start (e.g., re-insert dashes into the 32-hex trace ID), or (b) record securityTraceID as a span attribute (attribute.String("fullsend.security_trace_id", securityTraceID)) so the two domains remain joinable.

Medium

• [adr-immutability-violation] docs/ADRs/0050-distributed-tracing-instrumentation.md — ADR 0050 has status Accepted on main. This PR modifies its Decision section: removing run-summary.json from the Level 1 artifact description and changing the export model. Per AGENTS.md, accepted ADRs are point-in-time records — substantive changes require a new superseding ADR. The core decision (three-level opt-in, OTel-native, W3C propagation) is unchanged, but the Level 1 artifact specification is modified in place.
  Remediation: Either (a) create a superseding ADR documenting the run-summary.json removal and live OTLP export change, updating ADR 0050's status to Superseded, or (b) limit changes to a short annotation noting that run-summary.json was superseded by root-span attributes per this refactor.

• [behavioral-change] internal/telemetry/telemetry.go:54 — OTEL_SDK_DISABLED=true now suppresses Level 1 local file output entirely. The isSDKDisabled() check at line 54 returns a noop tracer and skips file creation. Previously, OTEL_SDK_DISABLED only affected the OTLP export path (Level 2); the bespoke recorder always wrote run-telemetry.jsonl. The distributed-tracing guide (updated in this PR) still states the file "is always written, even when no OTLP backend is configured" — but with OTEL_SDK_DISABLED=true, it is not written.
  Remediation: Move the isSDKDisabled() check to guard only the OTLP exporter creation (not the file exporter), or update docs/guides/infrastructure/distributed-tracing.md to note that OTEL_SDK_DISABLED=true suppresses all telemetry output including the local file.

• [edge-case] internal/telemetry/fileexporter.go:146 — convertValue for FLOAT64 emits v.AsFloat64() directly. If the value is NaN or ±Inf, json.Marshal returns an error. Unlike write errors (which set e.failed = true and disable the exporter), marshal errors are returned without setting e.failed, causing repeated error log lines on every subsequent span for the remainder of the run. This is an extremely unlikely scenario (OTel attributes would need to contain NaN/Inf), but the asymmetric error handling is a latent bug.
  Remediation: Guard FLOAT64 values: if math.IsNaN(f) || math.IsInf(f, 0), substitute 0.0 or a string representation. Alternatively, set e.failed = true in the marshal error path of ExportSpans to match the write error behavior.

Low

• [scope-creep-beyond-issue] docs/guides/dev/testing-agent-changes.md — New 147-line developer guide unrelated to the telemetry refactor. Issue Explore the OpenTelemetry Go SDK for Level 2 trace export #2780 scope is OTel SDK evaluation for trace export. Consider moving to a separate PR.

• [breaking-change-scope-alignment] internal/telemetry/recorder.go — Issue Explore the OpenTelemetry Go SDK for Level 2 trace export #2780 describes exploratory work ("Explore the OpenTelemetry Go SDK"). The PR implements a committed breaking change (removing run-summary.json, renaming the agent attribute to fullsend.agent). Verify that the breaking change has been authorized beyond the exploratory scope.

• [data-exposure] internal/cli/run.go — runErr.Error() is recorded in span status messages via rootSpan.SetStatus(codes.Error, runErr.Error()), agentSpan.SetStatus(codes.Error, ...), and sandboxSpan.SetStatus(codes.Error, ...). The old recorder only recorded fixed "ok"/"error" strings. Error messages may contain internal paths or API details. This is standard OTel practice and the data is metadata-class, but it is a net increase in information written to the telemetry file and (when configured) exported to a remote backend.

• [behavioral-change] internal/cli/run.go — The run directory is now created before the pre-script (step 3 → step 4), whereas previously the pre-script ran before directory creation. Pre-scripts now find the run directory already present. Risk is negligible since the run directory path (PID + timestamp) is unpredictable and not communicated to pre-scripts.

• [error-handling] internal/cli/run.go — tracingCleanup(flushCtx) in the defer block has no recover(). The old Recorder.emit() had an explicit defer/recover to ensure panics in the telemetry path never crashed the run. The OTel SDK is production-grade and unlikely to panic during shutdown, but wrapping in recover() would maintain the fail-open contract.

• [test-adequacy] internal/telemetry/telemetry_test.go — No end-to-end test exercises the unsampled inbound TRACEPARENT scenario through the W3C propagator + parentSampledProcessor pipeline. TestParentSampledProcessor_SuppressesEntireTrace validates the processor in isolation but does not cover the full Setup path with TRACEPARENT propagation.

• [test-adequacy] internal/telemetry/fileexporter_test.go — No concurrency tests for the fileExporter. The SimpleSpanProcessor serializes calls, but the mutex should still be tested under concurrent access to guard against future processor changes. The crash-safety concern from the prior review (no f.Sync()) is resolved.

Previous run (4)

Review

Re-review (prior: 230e3f3, provenance: app-verified). New commit 594cc9c — the sampling architecture changed from ParentBased(AlwaysSample()) to AlwaysSample() + a custom parentSampledProcessor. The fileExporter now includes f.Sync() per span (crash-safety concern from prior review addressed).

The change aligns with ADR 0050 and is authorized by issue #2780. All prior stale-doc findings remain addressed. Six of the seven prior LOW findings remain valid on unchanged code (severity-anchored). One new medium finding on the changed sampling logic.

Findings

Medium

• [logic-error] internal/telemetry/telemetry.go — parentSampledProcessor.OnEnd only filters spans whose immediate parent is remote and unsampled. With AlwaysSample() as the sampler, the root "run" span is correctly filtered when the inbound TRACEPARENT has flags=00, but child spans (sandbox_create, agent) have a local parent (the root span, which carries FlagsSampled because AlwaysSample() was used). For child spans psc.IsRemote() is false, so the condition short-circuits and the spans pass through to the OTLP batch processor. This violates the documented contract in docs/guides/infrastructure/distributed-tracing.md: "nothing is exported" when the upstream sampling decision says don't sample.
  Remediation: Replace the per-span check with a mechanism that propagates the root's sampling decision to all descendants. Options: (a) check whether the trace's root span has a remote unsampled parent by storing the decision at Setup/span-creation time and passing it through context, (b) use two TracerProviders — one with AlwaysSample() for the file exporter and one with ParentBased(AlwaysSample()) for OTLP, or (c) wrap the file exporter in an unconditional span processor and use ParentBased(AlwaysSample()) as the top-level sampler (noting that the file exporter's processor would need to handle non-sampled spans via a custom SpanProcessor.OnEnd that writes regardless of sampling state).

Low

• [behavioral-change] internal/cli/run.go — The run directory is now created before the pre-script (step 3 → step 4), whereas previously the pre-script ran before directory creation. Pre-scripts now find the run directory already present. Risk is negligible since the run directory path (PID + timestamp) is unpredictable and not communicated to pre-scripts.

• [error-handling] internal/cli/run.go — tracingCleanup(flushCtx) in the defer block has no recover(). The old Recorder.emit() had an explicit defer/recover to ensure panics in the telemetry path never crashed the run. The OTel SDK is production-grade and unlikely to panic during shutdown, but wrapping in recover() would maintain the fail-open contract.

• [test-adequacy] internal/telemetry/telemetry_test.go — No test exercises the unsampled inbound TRACEPARENT scenario (flags=00). The old suite had TestExportRunDir_UnsampledRunSkipsExport which validated that no spans reached the backend. Given the medium-severity parentSampledProcessor finding, a test specifically verifying that all spans (root + children) are suppressed from OTLP when the inbound trace is unsampled would catch the current regression.

• [test-adequacy] internal/telemetry/fileexporter_test.go — No concurrency tests for the new fileExporter. The old recorder_test.go had TestRecorder_ConcurrentWritesNoCorruption (20 goroutines). The current exporter is paired with SimpleSpanProcessor (single-threaded), but the mutex should still be tested under concurrent access to guard against future processor changes. The crash-safety concern from the prior review (no f.Sync()) is resolved — the current code calls f.Sync() after each write.

• [data-exposure] internal/cli/run.go — runErr.Error() is recorded in span status messages via rootSpan.SetStatus(codes.Error, runErr.Error()). The old recorder only recorded fixed "ok"/"error" strings. Error messages may contain internal paths or API details. This is standard OTel practice and the data is metadata-class, but it is a net increase in information written to the telemetry file and (when configured) exported to a remote backend.

• [naming-convention] internal/telemetry/trace.go — New function Traceparent does not follow the prior naming convention (TraceParent, TraceParentWithFlags). The old functions are fully removed so there is no runtime inconsistency, but the capitalization change diverges from the prior codebase pattern. The W3C spec uses "traceparent" as a single word, supporting the new convention.

• [edge-case] internal/telemetry/fileexporter.go — convertValue for FLOAT64 emits {"doubleValue": v.AsFloat64()}. If the value is NaN or ±Inf, json.Marshal will produce null (NaN) or fail (Inf), causing the entire ExportSpans call to fail and setting e.failed = true — which silently drops all subsequent spans for the remainder of the run. This is an extremely unlikely edge case (OTel attributes would need to contain NaN/Inf), but it would disable the file exporter entirely.
  Remediation: Guard FLOAT64 values: if math.IsNaN or math.IsInf, fall through to a string representation or substitute 0.0.

Previous run (5)

Review

Re-review (prior: 230e3f3, provenance: app-verified). Same commit — the PR transitioned from draft to non-draft and carries the ready-for-merge label.

No code changes since the prior review. All seven prior LOW findings remain valid and severity-anchored on unchanged code. The prior review's HIGH-severity AlwaysSample finding (from two reviews ago) remains resolved: Setup uses sdktrace.ParentBased(sdktrace.AlwaysSample()). All stale-doc findings from earlier reviews remain addressed.

The change aligns with ADR 0050's decision for "Framework-native OpenTelemetry" and is authorized by issue #2780 ("Telemetry phase 2 & 3 — OpenTelemetry Go SDK for trace export"). No scope creep. No security findings above LOW. No stale documentation references outside the diff.

Findings

Low

• [behavioral-change] internal/cli/run.go — The run directory is now created before the pre-script (step 3 → step 4), whereas previously the pre-script ran before directory creation. Pre-scripts now find the run directory already present. Risk is negligible since the run directory path (PID + timestamp) is unpredictable and not communicated to pre-scripts.

• [error-handling] internal/cli/run.go — tracingCleanup(flushCtx) in the defer block has no recover(). The old Recorder.emit() had an explicit defer/recover to ensure panics in the telemetry path never crashed the run. The OTel SDK is production-grade and unlikely to panic during shutdown, but wrapping in recover() would maintain the fail-open contract.

• [test-adequacy] internal/telemetry/telemetry_test.go — No test exercises the unsampled inbound TRACEPARENT scenario (flags=00). The ParentBased(AlwaysSample()) fix is correct and the SDK is well-tested, but a test pinning the invariant (span creation suppressed when parent is unsampled) would guard against regressions.

• [test-adequacy] internal/telemetry/telemetry_test.go — No concurrency or crash-safety tests for the new fileExporter. The old recorder_test.go had TestRecorder_ConcurrentWritesNoCorruption (20 goroutines) and TestRecorder_CrashSafety_LinesDurableWithoutFinalize. The new file exporter does not call f.Sync() after writes, unlike the old recorder — crash safety is reduced. For L1's handful of spans per run, per-span fsync is acceptable (the old code explicitly accepted this cost).

• [data-exposure] internal/cli/run.go — runErr.Error() is recorded in span status messages via rootSpan.SetStatus(codes.Error, runErr.Error()). The old recorder only recorded fixed "ok"/"error" strings. Error messages may contain internal paths or API details. This is standard OTel practice and the data is metadata-class, but it is a net increase in information written to the telemetry file and (when configured) exported to a remote backend.

• [naming-convention] internal/telemetry/trace.go — New function Traceparent does not follow the prior naming convention (TraceParent, TraceParentWithFlags). The old functions are fully removed so there is no runtime inconsistency, but the capitalization change diverges from the prior codebase pattern. The W3C spec uses "traceparent" as a single word, supporting the new convention.

• [edge-case] internal/telemetry/fileexporter.go — convertValue for FLOAT64 emits {"doubleValue": v.AsFloat64()}. If the value is NaN or ±Inf, json.Marshal will produce null (NaN) or fail (Inf), causing the entire ExportSpans call to fail. This is an extremely unlikely edge case (OTel attributes would need to contain NaN/Inf), but it would silently drop the batch.
  Remediation: Guard FLOAT64 values: if math.IsNaN or math.IsInf, fall through to a string representation.

Previous run (6)

Review

Re-review (prior: 230e3f3, provenance: app-verified). Same commit — the PR transitioned from draft to non-draft and carries the ready-for-merge label.

No code changes since the prior review. All seven prior LOW findings remain valid and severity-anchored on unchanged code. The prior review's HIGH-severity AlwaysSample finding (from two reviews ago) remains resolved: Setup uses sdktrace.ParentBased(sdktrace.AlwaysSample()). All stale-doc findings from earlier reviews remain addressed.

The change aligns with ADR 0050's decision for "Framework-native OpenTelemetry" and is authorized by issue #2780 ("Telemetry phase 2 & 3 — OpenTelemetry Go SDK for trace export"). No scope creep. No security findings above LOW. No stale documentation references outside the diff.

Findings

Low

• [behavioral-change] internal/cli/run.go — The run directory is now created before the pre-script (step 3 → step 4), whereas previously the pre-script ran before directory creation. Pre-scripts now find the run directory already present. Risk is negligible since the run directory path (PID + timestamp) is unpredictable and not communicated to pre-scripts.

• [error-handling] internal/cli/run.go — tracingCleanup(flushCtx) in the defer block has no recover(). The old Recorder.emit() had an explicit defer/recover to ensure panics in the telemetry path never crashed the run. The OTel SDK is production-grade and unlikely to panic during shutdown, but wrapping in recover() would maintain the fail-open contract.

• [test-adequacy] internal/telemetry/telemetry_test.go — No test exercises the unsampled inbound TRACEPARENT scenario (flags=00). The ParentBased(AlwaysSample()) fix is correct and the SDK is well-tested, but a test pinning the invariant (span creation suppressed when parent is unsampled) would guard against regressions.

• [test-adequacy] internal/telemetry/telemetry_test.go — No concurrency or crash-safety tests for the new fileExporter. The old recorder_test.go had TestRecorder_ConcurrentWritesNoCorruption (20 goroutines) and TestRecorder_CrashSafety_LinesDurableWithoutFinalize. The new file exporter does not call f.Sync() after writes, unlike the old recorder — crash safety is reduced. For L1's handful of spans per run, per-span fsync is acceptable (the old code explicitly accepted this cost).

• [data-exposure] internal/cli/run.go — runErr.Error() is recorded in span status messages via rootSpan.SetStatus(codes.Error, runErr.Error()). The old recorder only recorded fixed "ok"/"error" strings. Error messages may contain internal paths or API details. This is standard OTel practice and the data is metadata-class, but it is a net increase in information written to the telemetry file and (when configured) exported to a remote backend.

• [naming-convention] internal/telemetry/trace.go — New function Traceparent does not follow the prior naming convention (TraceParent, TraceParentWithFlags). The old functions are fully removed so there is no runtime inconsistency, but the capitalization change diverges from the prior codebase pattern. The W3C spec uses "traceparent" as a single word, supporting the new convention.

• [edge-case] internal/telemetry/fileexporter.go — convertValue for FLOAT64 emits {"doubleValue": v.AsFloat64()}. If the value is NaN or ±Inf, json.Marshal will produce null (NaN) or fail (Inf), causing the entire ExportSpans call to fail. This is an extremely unlikely edge case (OTel attributes would need to contain NaN/Inf), but it would silently drop the batch.
  Remediation: Guard FLOAT64 values: if math.IsNaN or math.IsInf, fall through to a string representation.

Previous run (7)

Review

Re-review (prior: cf802f1, provenance: app-verified). The PR is a draft.

The prior review's HIGH-severity AlwaysSample finding has been resolved: Setup now uses sdktrace.ParentBased(sdktrace.AlwaysSample()), which correctly respects the inbound TRACEPARENT's W3C sampling decision. When an upstream sends flags=00, the ParentBased sampler prevents span creation entirely — neither the file exporter nor the OTLP batch exporter receives spans.

The buildResourceSpans value-copy issue from two reviews ago was confirmed resolved in the prior review and remains correct: the pointer-based map accumulates all spans before dereferencing at output-assembly time.

All three prior stale-doc findings (docs/architecture.md, docs/guides/infrastructure/distributed-tracing.md, docs/ADRs/0050-distributed-tracing-instrumentation.md) remain addressed.

No new medium+ findings. The remaining findings are low-severity items from the prior review (severity anchored on unchanged code) plus one new edge-case finding.

Findings

Low

• [behavioral-change] internal/cli/run.go — The run directory is now created before the pre-script (step 3 → step 4), whereas previously the pre-script ran before directory creation. Pre-scripts now find the run directory already present. Risk is negligible since the run directory path (PID + timestamp) is unpredictable and not communicated to pre-scripts.

• [error-handling] internal/cli/run.go — tracingCleanup(flushCtx) in the defer block has no recover(). The old Recorder.emit() had an explicit defer/recover to ensure panics in the telemetry path never crashed the run. The OTel SDK is production-grade and unlikely to panic during shutdown, but wrapping in recover() would maintain the fail-open contract.

• [test-adequacy] internal/telemetry/telemetry_test.go — No test exercises the unsampled inbound TRACEPARENT scenario (flags=00). The ParentBased(AlwaysSample()) fix is correct and the SDK is well-tested, but a test pinning the invariant (span creation suppressed when parent is unsampled) would guard against regressions.

• [test-adequacy] internal/telemetry/telemetry_test.go — No concurrency or crash-safety tests for the new fileExporter. The old recorder_test.go had TestRecorder_ConcurrentWritesNoCorruption (20 goroutines) and TestRecorder_CrashSafety_LinesDurableWithoutFinalize. The new file exporter does not call f.Sync() after writes, unlike the old recorder — crash safety is reduced. For L1's handful of spans per run, per-span fsync is acceptable (the old code explicitly accepted this cost).

• [data-exposure] internal/cli/run.go — runErr.Error() is recorded in span status messages via rootSpan.SetStatus(codes.Error, runErr.Error()). The old recorder only recorded fixed "ok"/"error" strings. Error messages may contain internal paths or API details. This is standard OTel practice and the data is metadata-class, but it is a net increase in information written to the telemetry file and (when configured) exported to a remote backend.

• [naming-convention] internal/telemetry/trace.go — New function Traceparent does not follow the prior naming convention (TraceParent, TraceParentWithFlags). The old functions are fully removed so there is no runtime inconsistency, but the capitalization change diverges from the prior codebase pattern. The W3C spec uses "traceparent" as a single word, supporting the new convention.

• [edge-case] internal/telemetry/fileexporter.go — convertValue for FLOAT64 emits {"doubleValue": v.AsFloat64()}. If the value is NaN or ±Inf, json.Marshal will produce null (NaN) or fail (Inf), causing the entire ExportSpans call to fail. This is an extremely unlikely edge case (OTel attributes would need to contain NaN/Inf), but it would silently drop the batch.
  Remediation: Guard FLOAT64 values: if math.IsNaN or math.IsInf, fall through to a string representation.

Previous run (8)

Review

Re-review (prior: cf802f1, provenance: app-verified). The PR is a draft.

The prior review's HIGH-severity AlwaysSample finding has been resolved: Setup now uses sdktrace.ParentBased(sdktrace.AlwaysSample()), which correctly respects the inbound TRACEPARENT's W3C sampling decision. When an upstream sends flags=00, the ParentBased sampler prevents span creation entirely — neither the file exporter nor the OTLP batch exporter receives spans.

The buildResourceSpans value-copy issue from two reviews ago was confirmed resolved in the prior review and remains correct: the pointer-based map accumulates all spans before dereferencing at output-assembly time.

All three prior stale-doc findings (docs/architecture.md, docs/guides/infrastructure/distributed-tracing.md, docs/ADRs/0050-distributed-tracing-instrumentation.md) remain addressed.

No new medium+ findings. The remaining findings are low-severity items from the prior review (severity anchored on unchanged code) plus one new edge-case finding.

Findings

Low

• [behavioral-change] internal/cli/run.go — The run directory is now created before the pre-script (step 3 → step 4), whereas previously the pre-script ran before directory creation. Pre-scripts now find the run directory already present. Risk is negligible since the run directory path (PID + timestamp) is unpredictable and not communicated to pre-scripts.

• [error-handling] internal/cli/run.go — tracingCleanup(flushCtx) in the defer block has no recover(). The old Recorder.emit() had an explicit defer/recover to ensure panics in the telemetry path never crashed the run. The OTel SDK is production-grade and unlikely to panic during shutdown, but wrapping in recover() would maintain the fail-open contract.

• [test-adequacy] internal/telemetry/telemetry_test.go — No test exercises the unsampled inbound TRACEPARENT scenario (flags=00). The ParentBased(AlwaysSample()) fix is correct and the SDK is well-tested, but a test pinning the invariant (span creation suppressed when parent is unsampled) would guard against regressions.

• [test-adequacy] internal/telemetry/telemetry_test.go — No concurrency or crash-safety tests for the new fileExporter. The old recorder_test.go had TestRecorder_ConcurrentWritesNoCorruption (20 goroutines) and TestRecorder_CrashSafety_LinesDurableWithoutFinalize. The new file exporter does not call f.Sync() after writes, unlike the old recorder — crash safety is reduced. For L1's handful of spans per run, per-span fsync is acceptable (the old code explicitly accepted this cost).

• [data-exposure] internal/cli/run.go — runErr.Error() is recorded in span status messages via rootSpan.SetStatus(codes.Error, runErr.Error()). The old recorder only recorded fixed "ok"/"error" strings. Error messages may contain internal paths or API details. This is standard OTel practice and the data is metadata-class, but it is a net increase in information written to the telemetry file and (when configured) exported to a remote backend.

• [naming-convention] internal/telemetry/trace.go — New function Traceparent does not follow the prior naming convention (TraceParent, TraceParentWithFlags). The old functions are fully removed so there is no runtime inconsistency, but the capitalization change diverges from the prior codebase pattern. The W3C spec uses "traceparent" as a single word, supporting the new convention.

• [edge-case] internal/telemetry/fileexporter.go — convertValue for FLOAT64 emits {"doubleValue": v.AsFloat64()}. If the value is NaN or ±Inf, json.Marshal will produce null (NaN) or fail (Inf), causing the entire ExportSpans call to fail. This is an extremely unlikely edge case (OTel attributes would need to contain NaN/Inf), but it would silently drop the batch.
  Remediation: Guard FLOAT64 values: if math.IsNaN or math.IsInf, fall through to a string representation.

Previous run (9)

Review

Re-review (prior: 6ed0f8f, provenance: app-verified). The PR is a draft.

The prior review's medium-severity stale-doc findings (docs/architecture.md, docs/guides/infrastructure/distributed-tracing.md, docs/ADRs/0050-distributed-tracing-instrumentation.md) have been addressed — all three files are now updated in this PR. The buildResourceSpans value-copy concern from the prior review has also been resolved: the output construction now correctly dereferences after all spans are collected.

Findings

High

• [logic-error] internal/telemetry/telemetry.go:66 — Setup uses sdktrace.WithSampler(sdktrace.AlwaysSample()), which overrides the inbound TRACEPARENT's W3C sampling decision. When an upstream sends flags=00 (do not sample), the batch OTLP exporter will still export spans to the remote backend. This breaks the documented contract in docs/guides/infrastructure/distributed-tracing.md: "nothing is exported." The old architecture explicitly checked the sampled flag in otlp.ExportRunDir via summarySampled() and skipped export for unsampled traces. The local file exporter should always write (which is correct), but the OTLP exporter must respect the upstream sampling decision.
  Remediation: Replace sdktrace.AlwaysSample() with a sampler that respects parent-based sampling for the OTLP path. Options: (a) use sdktrace.ParentBased(sdktrace.AlwaysSample()) and make the file exporter unconditional (e.g., via a span processor that writes regardless of sampling), (b) use a custom composite sampler, or (c) configure two TracerProviders.

Low

• [behavioral-change] internal/cli/run.go:760 — The run directory is now created before the pre-script (step 3 → step 4), whereas previously the pre-script ran at step 2c before directory creation. Pre-scripts now find the run directory already present. Risk is negligible since the run directory path (PID + timestamp) is unpredictable and not communicated to pre-scripts.

• [error-handling] internal/cli/run.go:801 — tracingCleanup(flushCtx) in the defer block has no recover(). The old Recorder.emit() had an explicit defer/recover to ensure panics in the telemetry path never crashed the run. The OTel SDK is production-grade and unlikely to panic during shutdown, but wrapping in recover() would maintain the fail-open contract.

• [test-adequacy] internal/telemetry/telemetry_test.go — No test exercises the unsampled inbound TRACEPARENT scenario (flags=00). The old suite had TestExportRunDir_UnsampledRunSkipsExport. This gap is directly related to the HIGH AlwaysSample finding.

• [test-adequacy] internal/telemetry/telemetry_test.go — No concurrency or crash-safety tests for the new fileExporter. The old recorder_test.go had TestRecorder_ConcurrentWritesNoCorruption (20 goroutines) and TestRecorder_CrashSafety_LinesDurableWithoutFinalize. Additionally, the new file exporter does not call f.Sync() after writes, unlike the old recorder — crash safety is reduced.

• [data-exposure] internal/cli/run.go:441 — runErr.Error() is now recorded in span status messages via rootSpan.SetStatus(codes.Error, runErr.Error()). The old recorder only recorded fixed "ok"/"error" strings. Error messages may contain internal paths or API details. This is standard OTel practice and the data is metadata-class, but it is a net increase in information written to the telemetry file and (when configured) exported to a remote backend.

• [naming-convention] internal/telemetry/trace.go:31 — New function Traceparent does not follow the existing naming convention (TraceParent, TraceParentWithFlags). The old functions are fully removed so there is no runtime inconsistency, but the capitalization change diverges from the prior codebase pattern.

Previous run (10)

Review

Re-review (prior: 36e2ef8, provenance: app-verified). The PR is a draft.

Summary

Replaces the bespoke two-layer telemetry architecture (L1 NDJSON recorder + L2 OTLP replay) with the OTel Go SDK's TracerProvider. The fileExporter writes OTLP JSON to run-telemetry.jsonl synchronously; otlptracehttp exports to a remote backend when configured. Net −2,252 lines — the reduction comes from deleting the hand-rolled recorder, the replay package, and the replay hack tool. The OTel SDK handles trace identity, propagation, and export natively.

The code change is architecturally sound and consistent with ADR 0050's decision for "Framework-native OpenTelemetry." The fail-open guarantee is maintained: Setup returns a noop tracer on failure, and telemetry never affects the run outcome. However, several in-repo documentation files still reference run-summary.json (now eliminated) and the old two-layer architecture, which will mislead operators after merge.

Findings

Medium

• [behavioral-change] internal/cli/run.go — Pre-script execution order changed: sandbox directory creation (step 3) now runs before the pre-script (step 2c). The diff comment explains the motivation ("moved before pre-script so the tracer can write to runDir"). This is an intentional reordering, but pre-scripts that assumed they run before any sandbox directory exists will encounter a different environment. The step numbering in comments (2c after 3) is also misleading.
  Remediation: Verify no existing pre-scripts depend on running before the sandbox directory exists. Update step numbering to reflect actual execution order.

• [stale-doc] docs/guides/infrastructure/distributed-tracing.md — Multiple stale references to run-summary.json (lines 15, 53, 70, 108, 156) and the two-layer "replay" export architecture. After this PR, only run-telemetry.jsonl is produced locally, and OTLP export happens live via the SDK's batch processor — not via post-run replay. The operational guidance in this document will be incorrect after merge.
  Remediation: Update the guide to reflect: only run-telemetry.jsonl is produced; OTLP export is live (when OTEL_EXPORTER_OTLP_*ENDPOINT is set); no post-run replay step.

• [stale-doc] docs/architecture.md:240 — States "Every run produces run-telemetry.jsonl and run-summary.json locally." The latter is no longer produced.
  Remediation: Update to reference only run-telemetry.jsonl.

• [stale-doc] docs/ADRs/0050-distributed-tracing-instrumentation.md:67 — States both files are produced in the Level 1 baseline. While ADRs are historical records, this is an Accepted ADR that implementers reference — the stale description may cause confusion.
  Remediation: Add an annotation noting that run-summary.json was superseded by root-span attributes (per this refactor), or update the artifact description.

Low

• [logic-error] internal/telemetry/fileexporter.go — buildResourceSpans stores a value copy of *ss (the otlpScopeSpans pointer) into rs.ScopeSpans. When the same resource+scope key appears again, new spans are appended to the pointer in ssm, but the stale value copy in rs.ScopeSpans is never updated. This means only the first span per resource+scope survives in the output. Currently unreachable: the file exporter is wrapped in NewSimpleSpanProcessor, which calls ExportSpans with exactly one span at a time. If the processor is ever changed to batching, spans will be silently dropped.
  Remediation: Either store []*otlpScopeSpans in rs.ScopeSpans, or rebuild the output from ssm after the loop. Add a test with multiple spans sharing the same resource and scope.

• [error-handling] internal/cli/run.go — The deferred cleanup calls tracingCleanup(flushCtx) (which invokes TracerProvider.Shutdown) without a recover(). The old recorder had an explicit defer/recover in its emit() method. While the OTel SDK is unlikely to panic during shutdown, wrapping in recover() would maintain crash-safety parity.

• [data-exposure] internal/cli/run.go — runErr.Error() is now recorded in span status messages (via rootSpan.SetStatus(codes.Error, runErr.Error())). The old recorder only recorded "ok" or "error" strings. Error messages may contain internal paths or API error details. This is standard OTel practice and the data is metadata-class, but it is a net increase in information written to the telemetry file and (when configured) exported to a remote backend.

• [test-adequacy] internal/telemetry/fileexporter_test.go — No test exercises multiple spans with the same resource and instrumentation scope in a single ExportSpans call. This is the code path where the buildResourceSpans value-copy issue would manifest.

• [test-adequacy] internal/telemetry/telemetry_test.go — The deleted recorder_test.go covered crash safety, concurrent writes, mid-run write failure, and idempotent finalization. The new tests cover Setup variations but not concurrency or crash-safety properties. These are now partially delegated to the OTel SDK, but the custom fileExporter mutex should still be tested under concurrent access.

---

Labels: PR refactors internal telemetry subsystem

Previous run (11)

Review

Re-review (prior: 36e2ef8, provenance: app-verified). The PR is a draft.

Summary

Replaces the bespoke two-layer telemetry architecture (L1 NDJSON recorder + L2 OTLP replay) with the OTel Go SDK's TracerProvider. The fileExporter writes OTLP JSON to run-telemetry.jsonl synchronously; otlptracehttp exports to a remote backend when configured. Net −2,252 lines — the reduction comes from deleting the hand-rolled recorder, the replay package, and the replay hack tool. The OTel SDK handles trace identity, propagation, and export natively.

The code change is architecturally sound and consistent with ADR 0050's decision for "Framework-native OpenTelemetry." The fail-open guarantee is maintained: Setup returns a noop tracer on failure, and telemetry never affects the run outcome. However, several in-repo documentation files still reference run-summary.json (now eliminated) and the old two-layer architecture, which will mislead operators after merge.

Findings

Medium

• [behavioral-change] internal/cli/run.go — Pre-script execution order changed: sandbox directory creation (step 3) now runs before the pre-script (step 2c). The diff comment explains the motivation ("moved before pre-script so the tracer can write to runDir"). This is an intentional reordering, but pre-scripts that assumed they run before any sandbox directory exists will encounter a different environment. The step numbering in comments (2c after 3) is also misleading.
  Remediation: Verify no existing pre-scripts depend on running before the sandbox directory exists. Update step numbering to reflect actual execution order.

• [stale-doc] docs/guides/infrastructure/distributed-tracing.md — Multiple stale references to run-summary.json (lines 15, 53, 70, 108, 156) and the two-layer "replay" export architecture. After this PR, only run-telemetry.jsonl is produced locally, and OTLP export happens live via the SDK's batch processor — not via post-run replay. The operational guidance in this document will be incorrect after merge.
  Remediation: Update the guide to reflect: only run-telemetry.jsonl is produced; OTLP export is live (when OTEL_EXPORTER_OTLP_*ENDPOINT is set); no post-run replay step.

• [stale-doc] docs/architecture.md:240 — States "Every run produces run-telemetry.jsonl and run-summary.json locally." The latter is no longer produced.
  Remediation: Update to reference only run-telemetry.jsonl.

• [stale-doc] docs/ADRs/0050-distributed-tracing-instrumentation.md:67 — States both files are produced in the Level 1 baseline. While ADRs are historical records, this is an Accepted ADR that implementers reference — the stale description may cause confusion.
  Remediation: Add an annotation noting that run-summary.json was superseded by root-span attributes (per this refactor), or update the artifact description.

Low

• [logic-error] internal/telemetry/fileexporter.go — buildResourceSpans stores a value copy of *ss (the otlpScopeSpans pointer) into rs.ScopeSpans. When the same resource+scope key appears again, new spans are appended to the pointer in ssm, but the stale value copy in rs.ScopeSpans is never updated. This means only the first span per resource+scope survives in the output. Currently unreachable: the file exporter is wrapped in NewSimpleSpanProcessor, which calls ExportSpans with exactly one span at a time. If the processor is ever changed to batching, spans will be silently dropped.
  Remediation: Either store []*otlpScopeSpans in rs.ScopeSpans, or rebuild the output from ssm after the loop. Add a test with multiple spans sharing the same resource and scope.

• [error-handling] internal/cli/run.go — The deferred cleanup calls tracingCleanup(flushCtx) (which invokes TracerProvider.Shutdown) without a recover(). The old recorder had an explicit defer/recover in its emit() method. While the OTel SDK is unlikely to panic during shutdown, wrapping in recover() would maintain crash-safety parity.

• [data-exposure] internal/cli/run.go — runErr.Error() is now recorded in span status messages (via rootSpan.SetStatus(codes.Error, runErr.Error())). The old recorder only recorded "ok" or "error" strings. Error messages may contain internal paths or API error details. This is standard OTel practice and the data is metadata-class, but it is a net increase in information written to the telemetry file and (when configured) exported to a remote backend.

• [test-adequacy] internal/telemetry/fileexporter_test.go — No test exercises multiple spans with the same resource and instrumentation scope in a single ExportSpans call. This is the code path where the buildResourceSpans value-copy issue would manifest.

• [test-adequacy] internal/telemetry/telemetry_test.go — The deleted recorder_test.go covered crash safety, concurrent writes, mid-run write failure, and idempotent finalization. The new tests cover Setup variations but not concurrency or crash-safety properties. These are now partially delegated to the OTel SDK, but the custom fileExporter mutex should still be tested under concurrent access.

---

Labels: PR refactors internal telemetry subsystem

Previous run (12)

Review

Findings

Medium

• [behavioral-change] internal/cli/run.go — Pre-script execution order changed: the pre-script now runs AFTER sandbox creation (was BEFORE). The diff comment at step 3 says "moved before pre-script so the tracer can write to runDir." This changes the contract for pre-script authors — any pre-script that depends on running before the sandbox exists (e.g., host-side setup consumed by sandbox creation) will silently break. The step numbering is also misleading: step "2c" now appears after step "3."
  Remediation: Verify no existing pre-scripts depend on running before sandbox creation. If any do, restructure to initialize the tracer before the pre-script without reordering sandbox creation. Document the behavioral change.

• [stale-doc] docs/architecture.md:240 — States "Every run produces run-telemetry.jsonl and run-summary.json locally" — this PR eliminates run-summary.json as an artifact. The reference will be incorrect after merge.
  Remediation: Update to reflect that only run-telemetry.jsonl is produced; run metadata is now recorded as span attributes on the root span.

• [stale-doc] docs/guides/infrastructure/distributed-tracing.md — The distributed tracing guide describes the two-layer architecture (Level 1 local files including run-summary.json, Level 2 post-run OTLP replay export) with multiple references to run-summary.json (lines 15, 53, 70, 108, 156). After this PR, run-summary.json is no longer produced and OTLP export happens via the SDK batch processor at runtime, not via post-run replay.
  Remediation: Update the guide to reflect the new architecture: stdouttrace writes to run-telemetry.jsonl, otlptracehttp exports in parallel when configured, no post-run replay step.

Low

• [edge-case] internal/cli/run.go — securityTraceID is derived via telemetry.UUIDFromTraceID(rootTraceID) which returns an empty string for invalid input. While the OTel SDK always produces valid 32-hex trace IDs (making failure practically impossible), the previous code had an explicit fallback via security.GenerateTraceID(). No equivalent fallback exists in the new code.

• [error-handling-idiom] internal/cli/run.go — The deferred cleanup function calls tracingCleanup(flushCtx) without a recover(). The previous implementation (recorder.go emit()) had an explicit defer/recover that ensured panics in the telemetry path never crashed the run — the package comment called graceful degradation "a hard requirement." The OTel SDK's TracerProvider.Shutdown() and span operations are third-party code that could theoretically panic.
  Remediation: Wrap the deferred cleanup in a recover() to maintain crash-safety parity with the previous implementation.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:07 AM UTC · Completed 9:20 AM UTC
Commit: 6ed0f8f · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 9:42 AM UTC · Ended 9:42 AM UTC
Commit: 5cd495a · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 9:43 AM UTC · Completed 9:59 AM UTC
Commit: cf802f1 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:18 PM UTC · Completed 1:30 PM UTC
Commit: 230e3f3 · View workflow run →

[rh-hemartin]

cc @dhshah13 @ascerra

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 1:46 PM UTC · Ended 1:55 PM UTC
Commit: 5cd495a · View workflow run →

[qodo-code-review]

PR Summary by Qodo

refactor(telemetry): replace bespoke recorder with OTel Go SDK

✨ Enhancement 🧪 Tests 📝 Documentation ⚙️ Configuration changes 🕐 40+ Minutes

AI Description

• Replace NDJSON recorder + post-run replay with OpenTelemetry Go SDK tracing.
• Export spans at runtime to OTLP JSONL file and optional OTLP/HTTP backend.
• Move run summary fields onto the root span; propagate TRACEPARENT via SDK.

Diagram

graph TD
  A["CLI run"] --> B["telemetry.Setup"] --> C["OTel TracerProvider"] --> D["fileExporter"] --> E[("run-telemetry.jsonl")]
  C -->|"if endpoint set"| G["OTLP/HTTP exporter"] --> H[/"OTLP backend"/]
  A -->|"TRACEPARENT env"| I["Child scripts"]
  subgraph Legend
    direction LR
    _mod["Module"] ~~~ _file[("Artifact")] ~~~ _ext[/"External"/]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use a standard OTel exporter format for the local artifact (e.g., stdout)

• ➕ Avoids maintaining OTLP JSON marshaling and spec edge-cases
• ➕ Leverages upstream-supported encoding behavior
• ➖ Would likely change the on-disk artifact contract (not OTLP JSONL)
• ➖ Harder to replay into arbitrary OTLP backends without extra tooling

2. Write OTLP protobuf to disk instead of OTLP JSONL

• ➕ Reduces custom JSON correctness/encoding concerns
• ➕ Potentially smaller artifacts and faster writes
• ➖ Less human-inspectable than JSONL
• ➖ Requires changing replay tooling/docs and any consumers expecting JSONL

3. Keep the two-layer record+replay pipeline but swap replay to use the SDK

• ➕ Minimizes behavior change during the run (no live exporter)
• ➕ Keeps file schema decoupled from OTLP
• ➖ Retains complexity and failure modes of reconstruction/replay
• ➖ Continues to require bespoke schema + mapping logic

Recommendation: The chosen approach (SDK-owned spans with always-on local OTLP JSONL plus optional live OTLP/HTTP export) is the right long-term simplification: it eliminates replay, aligns with standard propagation, and keeps telemetry fail-open. The main strategic risk is owning a custom OTLP JSON encoder; if that becomes burdensome, consider switching the local artifact to OTLP protobuf or adopting an upstream file exporter when available.

Files changed (15) +1285 / -515

Enhancement (2) +527 / -0

fileexporter.goImplement OTLP JSONL SpanExporter +394/-0

Implement OTLP JSONL SpanExporter

• Introduces a custom 'sdktrace.SpanExporter' that writes OTLP JSON 'TracesData' (hex-encoded IDs) as JSONL, including resources, scopes, spans, events, links, and status conversion.

internal/telemetry/fileexporter.go

telemetry.goAdd telemetry.Setup() to configure OTel provider + exporters +133/-0

Add telemetry.Setup() to configure OTel provider + exporters

• Adds 'Setup(dir, version)' to create a TracerProvider with a synchronous file exporter and optional OTLP/HTTP batch exporter when configured. Validates endpoint/protocol and returns a noop tracer on setup failure to keep runs unaffected.

internal/telemetry/telemetry.go

Refactor (2) +110 / -221

run.goReplace recorder + replay with OTel SDK spans and root-span summary attrs +106/-123

Replace recorder + replay with OTel SDK spans and root-span summary attrs

• Switches run telemetry to an SDK tracer from 'telemetry.Setup()', extracts inbound W3C context via the SDK propagator, propagates TRACEPARENT to child scripts, and records run/agent/sandbox spans with OTel attributes and status. Derives the security UUID from the SDK-generated trace ID and flushes tracing on shutdown with a bounded timeout.

internal/cli/run.go

trace.goReplace bespoke traceparent parsing with SpanContext formatting +4/-98

Replace bespoke traceparent parsing with SpanContext formatting

• Removes hand-rolled trace/span ID generation and traceparent parsing helpers and replaces them with 'Traceparent(trace.SpanContext)' for building the header from SDK span contexts; keeps UUID derivation helper.

internal/telemetry/trace.go

Tests (5) +528 / -267

scan_output_telemetry_test.goUpdate redaction test to skip only telemetry JSONL +6/-12

Update redaction test to skip only telemetry JSONL

• Updates the scan/redaction test to no longer expect 'run-summary.json' and to assert only 'run-telemetry.jsonl' is skipped while normal files are still sanitized.

internal/cli/scan_output_telemetry_test.go

telemetry_run_test.goUpdate telemetry tests for SDK-owned trace IDs and attribute API +26/-112

Update telemetry tests for SDK-owned trace IDs and attribute API

• Updates trace ID unification expectations to UUID-from-W3C (SDK-owned trace IDs), removes the old trace identity resolver tests, and updates span attribute helpers to return '[]attribute.KeyValue'.

internal/cli/telemetry_run_test.go

fileexporter_test.goAdd exhaustive tests for OTLP JSONL encoding +286/-0

Add exhaustive tests for OTLP JSONL encoding

• Adds tests covering resource/scope grouping, all attribute value types, events/links/status encoding, flags, edge cases, and ensures trace/span IDs are hex (not base64).

internal/telemetry/fileexporter_test.go

telemetry_test.goTest Setup() behavior, gating, and fail-open semantics +200/-0

Test Setup() behavior, gating, and fail-open semantics

• Verifies file export works, setup failures yield noop tracers, OTLP disable switches are honored, endpoint/protocol validation skips exporter creation, and the OTLP exporter seam is invoked when expected.

internal/telemetry/telemetry_test.go

trace_test.goRemove tests for deleted trace helpers; validate Traceparent() +10/-143

Remove tests for deleted trace helpers; validate Traceparent()

• Deletes coverage for removed ID generation/parsing helpers and adds a focused test for the new 'Traceparent()' output while retaining UUID derivation checks.

internal/telemetry/trace_test.go

Documentation (3) +23 / -25

0050-distributed-tracing-instrumentation.mdUpdate ADR to reflect single telemetry artifact +3/-3

Update ADR to reflect single telemetry artifact

• Updates ADR 0050 references from 'run-summary.json' to 'run-telemetry.jsonl' for the Level 1 baseline and related guidance.

docs/ADRs/0050-distributed-tracing-instrumentation.md

architecture.mdRefresh architecture docs for live OTLP export +1/-1

Refresh architecture docs for live OTLP export

• Updates the distributed tracing section to describe a single local telemetry file and optional live OTLP export.

docs/architecture.md

distributed-tracing.mdRewrite tracing guide for SDK-based telemetry +19/-21

Rewrite tracing guide for SDK-based telemetry

• Replaces the prior record+replay description with SDK live export semantics, crash behavior, and root-span metadata replacing 'run-summary.json'. Clarifies that 'run-telemetry.jsonl' remains the metadata-only baseline artifact.

docs/guides/infrastructure/distributed-tracing.md

Other (3) +97 / -2

go.modAdjust dependency graph after removing replay tooling +2/-2

Adjust dependency graph after removing replay tooling

• Moves 'go.opentelemetry.io/proto/otlp' to indirect and marks protobuf as indirect, reflecting the new SDK-based approach and removed replay packages.

go.mod

upload-traces-otelcol-config.yamlAdd otelcol config to replay OTLP JSONL +15/-0

Add otelcol config to replay OTLP JSONL

• Adds an OpenTelemetry Collector config that reads OTLP JSONL trace files and exports them to an OTLP/HTTP endpoint specified via env vars.

hack/upload-traces-otelcol-config.yaml

upload-traces.shAdd trace upload script using otelcol-contrib +80/-0

Add trace upload script using otelcol-contrib

• Adds a helper script to replay 'run-telemetry.jsonl' files from a file/dir/glob into any OTLP/HTTP backend via 'otelcol-contrib'. Validates inputs and prerequisites and passes include/endpoint via environment.

hack/upload-traces.sh

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Tickets: 🎫 Explore the OpenTelemetry Go SDK for Level 2 trace export

✅ Compliance rules (platform): 54 rules

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

1. ADR 0050 decision rewritten 📜 Skill insight ≡ Correctness

Description

The accepted ADR 0050 was substantively changed (e.g., Level 1 artifacts now omit
run-summary.json), which rewrites historical decision content instead of superseding it with a new
ADR. This violates the rule that accepted ADRs should not have their decision content rewritten.

Code

docs/ADRs/0050-distributed-tracing-instrumentation.md[R67-68]

+- Every run produces `run-telemetry.jsonl` in the output directory (uploaded as
+  GHA artifacts alongside transcripts)

Relevance

⭐⭐⭐ High

Repo policy discourages rewriting Accepted ADRs; prefers superseding ADRs (see ADR immutability
updates).

PR-#1966
PR-#1982

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

ADR 0050 is in Accepted status, but the PR changes the Level 1 decision text to remove
run-summary.json from the decided artifacts, which is a substantive edit to an accepted ADR’s
decision content.

Rule 1062058: Supersede accepted ADRs with new ADRs instead of modifying history
docs/ADRs/0050-distributed-tracing-instrumentation.md[63-70]
Skill: writing-adrs

Agent prompt

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

## Issue description
`docs/ADRs/0050-distributed-tracing-instrumentation.md` is `Accepted` but its substantive Decision content was edited (changing the decided Level 1 artifacts). Accepted ADRs must not be rewritten; instead, create a new ADR that supersedes the old one and only add an allowed supersedence note/status update to the original.

## Issue Context
This PR updates ADR 0050 to match the new OpenTelemetry SDK implementation, but that changes the historical record of what was originally decided.

## Fix Focus Areas
- docs/ADRs/0050-distributed-tracing-instrumentation.md[63-70]

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

---

2. Invalid noop trace propagation ✓ Resolved 🐞 Bug ≡ Correctness

Description

runAgent derives securityTraceID and TRACEPARENT from rootSpan.SpanContext() without
checking validity; when telemetry.Setup returns a noop tracer, the span context is invalid but its
derived values are still used for propagation and sandbox injection. This can result in
malformed/empty trace identity and breaks the intended “telemetry disabled ⇒ omit TRACEPARENT”
behavior for child scripts and downstream processes.

Code

internal/cli/run.go[R772-783]

+	tracer, tracingCleanup := telemetry.Setup(runDir, Version())
+	ctx = propagation.TraceContext{}.Extract(ctx, propagation.MapCarrier{
+		"traceparent": os.Getenv("TRACEPARENT"),
+		"tracestate":  os.Getenv("TRACESTATE"),
+	})
+	ctx, rootSpan := tracer.Start(ctx, "run", trace.WithAttributes(
+		attribute.String("fullsend.agent", agentName),
+		attribute.String("fullsend.work_item_id", workItemID),
+	))
+	rootTraceID := rootSpan.SpanContext().TraceID().String()
+	securityTraceID := telemetry.UUIDFromTraceID(rootTraceID)
+	traceparent := telemetry.Traceparent(rootSpan.SpanContext())

Relevance

⭐⭐⭐ High

Recent merged telemetry PRs tightened traceparent adoption/propagation; team treats this as
correctness-critical.

PR-#2960
PR-#3903

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The repo’s own tests show the noop path yields an invalid span context, while runAgent still
unconditionally formats/extracts IDs from that context and childScriptEnv will propagate any
non-empty TRACEPARENT string.

internal/telemetry/telemetry_test.go[38-45]
internal/cli/run.go[763-784]
internal/telemetry/trace.go[5-18]
internal/cli/run.go[1920-1939]

Agent prompt

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

### Issue description
`internal/cli/run.go` unconditionally derives `securityTraceID` and `traceparent` from the SDK span context. When `telemetry.Setup` falls back to a noop tracer, spans have an invalid span context; in that case, `runAgent` should **not** propagate a trace context derived from it and should still produce a valid security correlation ID.

### Issue Context
- `telemetry.Setup` explicitly returns a noop tracer on file-open failure; tests confirm spans from that tracer have an invalid span context.
- `childScriptEnv` only omits TRACEPARENT when the passed `traceparent` string is empty.

### Fix Focus Areas
- internal/cli/run.go[772-784]
- internal/cli/run.go[1920-1939]
- internal/telemetry/trace.go[5-18]

### Proposed fix
1. After `rootSpan` is created, validate `sc := rootSpan.SpanContext()`.
  - If `!sc.IsValid()`: set `traceparent = ""` (so child scripts don’t receive a bogus TRACEPARENT) and set `securityTraceID = security.GenerateTraceID()`.
  - Else: derive `securityTraceID` from the trace ID as today; if the derivation returns `""`, fall back to `security.GenerateTraceID()`.
2. Make `telemetry.Traceparent(sc)` defensive: return `""` when `!sc.IsValid()` (so callers don’t have to remember this rule everywhere).
3. Add/adjust a unit test to cover the noop-tracer path ensuring `traceparent == ""` and `securityTraceID` is non-empty + shell-safe.

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

---

3. PR misses ADR edit 📘 Rule violation § Compliance

Description

An accepted ADR (docs/ADRs/0050-distributed-tracing-instrumentation.md) is modified in this PR,
but the PR description does not explicitly call out that the accepted ADR was edited and what
changed. This violates the requirement to mention accepted ADR edits in the PR description.

Code

docs/ADRs/0050-distributed-tracing-instrumentation.md[R67-68]

+- Every run produces `run-telemetry.jsonl` in the output directory (uploaded as
+  GHA artifacts alongside transcripts)

Relevance

⭐⭐ Medium

No historical evidence found requiring PR descriptions to explicitly summarize Accepted ADR edits.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The diff shows an edit to the Decision section of an accepted ADR, which triggers the requirement
that the PR description explicitly mention the ADR edit and summarize it.

Rule 1062059: Call out edits to accepted ADRs in PR descriptions
docs/ADRs/0050-distributed-tracing-instrumentation.md[63-70]

Agent prompt

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

## Issue description
The PR edits an already-`Accepted` ADR but the PR description does not explicitly call out the ADR edit (ADR id/filename + short summary of what changed).

## Issue Context
This is required for auditability when accepted architecture decisions are modified.

## Fix Focus Areas
- docs/ADRs/0050-distributed-tracing-instrumentation.md[63-70]

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

---

View more (1)

4. distributed-tracing.md wrong directory 📜 Skill insight ⌂ Architecture

Description

The modified guide file is located at docs/guides/infrastructure/distributed-tracing.md, but
guides must live under either docs/guides/admin/ or docs/guides/user/. This violates the
documentation guide placement requirement.

Code

docs/guides/infrastructure/distributed-tracing.md[R10-18]

+Every `fullsend run` produces one file in the run output directory with no
configuration required:

-- **`run-telemetry.jsonl`** — NDJSON stream of lifecycle events (step starts,
-  completions, failures, warnings) with timestamps, durations, and trace IDs.
-- **`run-summary.json`** — Aggregated run summary including agent name, exit
-  code, step timings, total duration, and a W3C `traceparent` value for
-  downstream correlation.
+- **`run-telemetry.jsonl`** — OTLP JSON spans covering the run lifecycle
+  (sandbox creation, agent iterations, validation) with timestamps, durations,
+  trace IDs, and token/cost attributes.

-These files are always written, even when no OTLP backend is configured. They
-contain metadata only — no prompts, completions, or source code content.
+This file is always written, even when no OTLP backend is configured. It
+contains metadata only — no prompts, completions, or source code content.

Relevance

⭐⭐ Medium

No repo history found enforcing guides-only under admin/user; structure exists but placement
enforcement unclear.

PR-#332

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR modifies a file under docs/guides/ that is not in an admin/ or user/ subdirectory,
which is explicitly disallowed by the guide placement rule.

docs/guides/infrastructure/distributed-tracing.md[1-18]
Skill: writing-user-docs

Agent prompt

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

## Issue description
`docs/guides/infrastructure/distributed-tracing.md` is a guide outside the allowed `docs/guides/admin/` or `docs/guides/user/` directories.

## Issue Context
Per the docs guide structure rules, each guide must be placed under exactly one of the two allowed audience directories.

## Fix Focus Areas
- docs/guides/infrastructure/distributed-tracing.md[1-20]

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

---

5. Silent tracing cleanup failures 🐞 Bug ◔ Observability

Description

telemetry.Setup’s cleanup function discards errors from TracerProvider.Shutdown (OTLP batch
flush) and from closing the telemetry file, so export/flush failures can be silently lost. This
makes “best-effort telemetry” failures difficult to detect and debug when traces are missing.

Code

internal/telemetry/telemetry.go[R78-81]

+	cleanup := func(ctx context.Context) {
+		_ = tp.Shutdown(ctx)
+		_ = f.Close()
+	}

Relevance

⭐⭐ Medium

Team sometimes asks to surface swallowed errors, but handling cleanup Shutdown/Close errors isn’t
consistently enforced.

PR-#2370
PR-#320

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The cleanup closure explicitly assigns both shutdown/close results to _, and runAgent calls this
cleanup without any logging, so failures during flush/close are currently undetectable from logs.

internal/telemetry/telemetry.go[75-83]
internal/cli/run.go[810-815]

Agent prompt

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

### Issue description
The tracing cleanup path ignores errors from `tp.Shutdown(ctx)` and `f.Close()`. OTLP batcher shutdown errors can mean spans weren’t exported; close errors can indicate persistence problems.

### Issue Context
The PR’s stated policy is fail-open (telemetry must not affect the run outcome), but it’s still useful to surface cleanup errors as warnings.

### Fix Focus Areas
- internal/telemetry/telemetry.go[51-84]
- internal/cli/run.go[810-815]

### Proposed fix
- Change `Setup` to return a cleanup function that returns an `error` (e.g., `func(context.Context) error`), and in `runAgent` call it and `printer.StepWarn(...)` on error.
 - If you want to keep the signature, add a package-level optional warning hook (set by CLI) and call it when shutdown/close fails.
- When both shutdown and close fail, join them (e.g., `errors.Join`) so callers can log the combined failure.

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

---

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 11:37 AM UTC · Completed 11:55 AM UTC
Commit: d9c4077 · View workflow run →

[waynesun09]

Review Squad — 3 agents (Claude × 2, Grok) · round 3 at head d9c4077d

2 HIGH · 9 MEDIUM posted inline (11 unique findings after dedup and verification against prior rounds).

All 15 findings from the two prior review rounds are confirmed resolved or settled at head — not re-flagged. Today's bot finding (a) (OTEL_SDK_DISABLED suppressing Level 1 file) is now resolved via doc alignment. Bot finding (b) (NaN/Inf float marshal) remains unfixed and is re-flagged above at upgraded severity.

The two HIGHs are both trace-propagation regressions where deleted tests used to pin the contract — the test-coverage gap (flagged inline on telemetry_run_test.go) explains why they slipped through.

Assisted-by: Claude (review), Grok (review)

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 1:25 PM UTC · Completed 1:43 PM UTC
Commit: 33a9b65 · View workflow run →

[waynesun09]

Review Squad — round 4 re-review at 33a9b65b · APPROVED

10 of 11 round-3 findings verified fixed — both HIGHs (trace propagation) well-designed with TraceparentWithFlags + parentSampledProcessor. One surviving MEDIUM posted inline: --header flags silently dropped because the otelcol config doesn't reference OTEL_EXPORTER_OTLP_HEADERS. Non-blocking — the core telemetry migration is solid.

Assisted-by: Claude (review), Grok (review)

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 3:03 PM UTC · Completed 3:19 PM UTC
Commit: 33a9b65 · View workflow run →

[fullsend-ai-retro]

Retro: PR #4510 — refactor(telemetry)!: replace bespoke recorder with OTel Go SDK

PR #4510 replaced the two-layer telemetry system (hand-rolled NDJSON recorder + post-run OTLP replay) with the OpenTelemetry Go SDK. It was a significant refactor: 22 files changed, +1,635/−2,851 lines, breaking change. Authored by rh-hemartin, reviewed by waynesun09 (human, using a 3-agent review squad), fullsend-ai-review[bot], and qodo-code-review[bot]. Merged after 4 review rounds over 4 days.

Workflow metrics

• 111 total CI runs on the branch (37 fullsend dispatch, 22 functional tests, 22 e2e, 14 CI, 14 build-site)
• 14 successful review dispatches (one per push, with cancel-in-progress dedup)
• 2 failed dispatch runs — both transient GitHub infrastructure errors (HTTP 500, Service Unavailable), not agent bugs
• 16 no-op dispatch runs from pull_request_review events that correctly matched no stage (by design)
• 4 review rounds before human approval; 28+ findings across rounds 1–3

Key quality finding

The review bot's most significant gap was failure to analyze deleted test invariants for regression detection. The PR deleted ~1,540 lines of tests across 4 test files. The bot catalogued these deletions as LOW-severity "test-adequacy" notes but never connected the deleted assertions to behavioral regressions in the replacement code. The human reviewer (waynesun09) caught 4 HIGH-severity correctness bugs — span mis-parenting, all-zero TRACEPARENT injection, sampling flag resurrection, GenAI attribute dropping — all of which were pinned by the deleted tests. The bot also systematically under-rated severity in the 5 cases where both reviewers found the same issue, rating 1–2 levels lower than the human.

Overall: 12 unique findings from the human reviewer that the bot missed entirely, vs. 1 unique finding from the bot (security trace ID decoupling). The bot's strength was persistence — tracking findings across 12 iterations. Its weakness was behavioral contract analysis on large refactors.

Proposals filed

3 proposals targeting review agent capabilities and autonomy readiness tracking.

Proposals filed

• Review agent should analyze deleted test assertions to detect behavioral regressions in replacement code (in fullsend-ai/fullsend)
• Review agent should apply elevated severity when findings co-occur with breaking-change indicators (in fullsend-ai/fullsend)
• Counter-evidence for review autonomy: review agent missed 12 unique findings on large telemetry refactor PR #4510 (in fullsend-ai/fullsend)