fix(#5361): set span status from run outcome, not runErr alone - #5944
Conversation
E2E tests did not runThe
See E2E testing guide for details. |
PR Summary by QodoFix OTel span status to reflect run outcome (incl. transcript errors)
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Site previewPreview: https://f677ef58-site.fullsend-ai.workers.dev Commit: |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Automated review pass (3 findings).
|
All three confirmed and fixed in 92505a6. Sanitization: correct — the transcript fields reach a new export sink here and had none. Missing event on the transcript path: correct, and the PR description's "nothing is lost" was wrong as written. UTF-8 on the short path: correct — the guard was conditional on length, which defeats its stated purpose. Note |
92505a6 to
435c285
Compare
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Automated review pass (1 finding).
|
Confirmed and fixed. 256 was an invented conservative number — no OTel, collector, or MLflow limit motivates it, and the PR shouldn't imply one exists. Adopted your alignment suggestion: |
ac19a07 to
029a88b
Compare
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass: 2 findings verified against the current head commit (029a88b).
|
Both findings fixed in de43047, and the PR description is updated to describe the shipped head. Double truncation: confirmed — your arithmetic is right (up to 2,015 parser bytes + 18-byte prefix > 2,000), and the comment stated an invariant that was false on exactly the motivating path. The cap is now a true total bound: Stale description: updated — the deliberate-decisions bullet now states the 2,000-byte bound and its rationale. Auditing the claims for this round also surfaced a sibling gap now fixed in the same commit: the sandbox-create span exported an unbounded, unrepaired status built from an error that embeds raw supervisor/gateway/container logs — by this PR's own argument, one invalid byte there fails proto marshaling of the batch carrying every other span. It now gets the same treatment ( |
ee4d683 to
dd4b298
Compare
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Automated review pass (2 findings).
252b78f to
bbb8ddd
Compare
waynesun09
added
the
ok-to-test
…lone Agent and root spans previously reported Status Ok whenever runErr was nil, so failed runs exported as successes: the agent span ended before the fullsend-ai#2786 is_error transcript check could flip the outcome, and a harness without a validation loop returned nil alongside a non-zero agent exit. - Finalize the agent span after the transcript check via finalizeAgentSpan: a runtime error, transcript-reported error, or non-zero exit is Status Error. exit_code keeps the raw process exit; fullsend.transcript_error marks the override; RecordError attaches the exception event. - Key the root span status on the run outcome: validation passed is Ok (validation, not the last agent exit, is the success gate); otherwise a non-zero telemetryExitCode is Error even when runErr is nil. - Cap status descriptions at 256 bytes on a UTF-8 rune boundary — an invalid-UTF-8 status fails proto marshaling of the entire OTLP batch. Signed-off-by: Dharit Shah <dhshah@redhat.com>
…TF-8 unconditionally Addresses review findings on the span-status fix: - Transcript error text is agent-controlled and now reaches a new export sink (span status). Route it through the same sanitization the GHA annotation path applies, via a new exported SanitizeOutput wrapper, so ANSI escapes and workflow-command markers cannot reach telemetry backends. - Record the untruncated transcript error as a span event, matching the runtime-error path — the 256-byte status cap no longer loses verbose API error payloads. - Repair invalid UTF-8 for every status description, not only those long enough to trigger truncation: a short malformed message would otherwise reach the exporter and fail proto marshaling of the whole batch. Signed-off-by: Dharit Shah <dhshah@redhat.com>
…tatus descriptions RecordError feeds the same OTLP proto marshal path as SetStatus, so an invalid-UTF-8 error message fails export of the whole batch — the exact failure mode the previous commit fixed for status descriptions, left open on the exception-event path. Route both RecordError call sites through a recordSanitizedError helper that repairs the message first. Signed-off-by: Dharit Shah <dhshah@redhat.com>
…error precedent 256 was an invented number with no cited constraint. No OTel, collector, or backend limit mandates a specific value, so adopt the repo's existing bound for the same class of consumer-facing error text — maxTranscriptErrorLength's 2000 bytes — which also means a parse-time-truncated transcript message is never truncated twice. The comment now states the constraint honestly. Signed-off-by: Dharit Shah <dhshah@redhat.com>
… close the sandbox-status gap Review round: the cap comment claimed a parse-time-truncated transcript message is never truncated twice — false by arithmetic (truncateError emits up to 2,015 bytes, the status prefix adds 18). The cap is now a true total bound: truncateStatusMsgTo budgets the transcript payload for its prefix, the ellipsis counts against the limit, and messages within the 1,982-byte headroom pass through untouched. Boundary test added at the parser's maximum. Audit fallout, same defect class: the sandbox-create span exported an unbounded, unrepaired status built from an error embedding raw supervisor/gateway/container logs — one invalid byte there fails proto marshaling of the batch carrying every other span. It now gets the sibling treatment (recordSanitizedError + bounded status), which also makes the cap comment's every-error-status claim true repo-wide. The straddle test moved to the actual cut point (cap minus ellipsis) — at the old offset the walk-back was no longer exercised. Signed-off-by: Dharit Shah <dhshah@redhat.com>
…ript console line Two findings from the round-5 review pass: - recordSanitizedError bounds the exception message at maxSpanEventMsgLen (8192) on top of UTF-8 repair. The sandbox-create error embeds raw supervisor/gateway logs collected with no line limit, the SDK never truncates event attribute values, and the same text rides the root span via the wrapped runErr — an oversized batch can be rejected by the collector whole. - The transcript-failure console line prints the same bounded, sanitized string the span event records, via TranscriptError.DisplayMessage — one rendering shared with the GHA annotation path. Console behavior changes: an empty ErrorMessage prints the subtype fallback instead of a bare trailing colon, multi-line messages are flattened, and :: markers are broken — a raw transcript payload could otherwise start a ::workflow-command:: line in the CI job log. The bound also covers Subtype, which unlike ErrorMessage is not parser-truncated. Signed-off-by: Dharit Shah <dhshah@redhat.com>
…ute values Closes the two remaining unbounded telemetry inputs adjacent to the round-5 fixes: - TranscriptError.DisplayMessage bounds the subtype fallback with the same truncateError treatment ErrorMessage gets at parse time — the transcript parser never truncates Subtype and accepts 1MB lines, so the fallback could otherwise reach the console line, the span sinks, and the GHA annotation path unbounded. - telemetry.Setup configures provider span limits: attribute values are SDK-truncated (UTF-8-safely) at 8192 bytes, matching the exception-event bound, unless the operator sets OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT or OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT. Free-text attributes (a transcript-derived model name, a pre-script skip reason) were otherwise unbounded. The limit does not apply to event attributes, which recordSanitizedError already bounds. Signed-off-by: Dharit Shah <dhshah@redhat.com>
…d drop the unused wrapper - spanLimits treated the SDK's -1 as 'operator did not configure', but -1 is also the OTel sentinel an operator sets explicitly for unlimited — the two collapse to the same struct value in NewSpanLimits. The env vars are now consulted directly (parsed the way the SDK parses them), so any parseable operator setting, including -1, is honored as-is and the 8192 default applies only when neither variable is set. - The exported runtime.SanitizeOutput wrapper lost its last production caller when the console line moved to TranscriptError.DisplayMessage; removed until a real external consumer lands. Signed-off-by: Dharit Shah <dhshah@redhat.com>
… pin every span finalizer - attrValueLenConfigured returned true if either env var parsed, but the SDK's firstInt short-circuits on the first non-empty variable — an unparseable specific var plus a valid generic var left attribute values unbounded. The helper now mirrors firstInt exactly (first non-empty key decides alone), with the SDK's own NewSpanLimits output asserted as ground truth in the test so the two cannot drift silently. - The sandbox-create and root spans finalize through extracted, tested helpers (finalizeSandboxSpan, finalizeRootSpan), matching the agent span: all three spans now pin the exception event, bounded repaired status, and end-exactly-once on the wire via SpanRecorder tests. - Recorder tests pin the OTEL span-limit env vars so an ambient OTEL_SPAN_EVENT_COUNT_LIMIT on a CI runner cannot drop the exception events they assert on. Signed-off-by: Dharit Shah <dhshah@redhat.com>
…e 8192 bounds
- sanitize()'s single ReplaceAll pass reconstituted a literal "::" at
the seam of two replacements ("::::" -> ": :: :"), so a crafted colon
run survived the GHA command-injection guard. Colon pairs now break to
a fixed point — no output contains "::", re-sanitizing is a no-op, and
at most two passes run. Pinned by exact-output tests for 3-6 colon
runs and an idempotence property test over both sanitize variants.
Worst-case sanitization growth becomes just under 2x (4,014 bytes for
the parser-max transcript message), still well inside the event bound.
- maxSpanEventMsgLen is now defined from the exported
telemetry.MaxSpanAttrValueLen, so the two 8,192 defaults cannot drift;
the comments on both sides state the shared-default relationship and
the deliberate non-tracking of runtime attribute overrides.
Signed-off-by: Dharit Shah <dhshah@redhat.com>
…an-limit tests hermetic - ADR 0050's Level 1 'errors' bullet now states what error text covers: bounded, repaired, sanitized-where-agent-controlled operational error text on statuses and exception events, including the container/ supervisor log excerpt a failed sandbox create embeds. The Level 3 gate governs prompt/completion capture, which is separate from error reporting. The same excerpt was exported unbounded on the sandbox status before this branch bounded both channels; finalizeSandboxSpan's comment records the classification. - pinOTELEnv clears OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT and OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT, which spanLimits made load-bearing, so every telemetry test is hermetic against ambient runner env — TestSetup_SpanAttributeValueLengthLimit failed under an ambient 512-byte override before this. Signed-off-by: Dharit Shah <dhshah@redhat.com>
…e and state the SDK's units - The SDK's attribute limit repairs invalid UTF-8 only when it truncates — a value at or under the limit passes through untouched, and one invalid byte fails proto marshaling of the whole OTLP batch, the exact failure mode statuses and events already guard against. Every dynamic string attribute (agent name, work-item id, model, skip reason, system, security trace id) now goes through stringAttr, which repairs to valid UTF-8; literal values keep attribute.String. - The SDK counts attribute characters, not bytes: a multibyte value can reach four bytes per character on the wire, and the event-message bound counts bytes. Comments on both constants now state the units and that the coupling is a shared numeric default, each side applying it in its own unit. - TestAttrLimit_SDKBehaviorCanary pins both SDK properties (character counting, no under-limit repair) against the live SDK, so an upgrade that changes either fails the suite instead of silently shifting the export contract. Signed-off-by: Dharit Shah <dhshah@redhat.com>
cd273a6 to
3e1ac84
Compare
github-actions
Bot
removed
the
ok-to-test
|
🤖 Finished Retro · ✅ Success · Started 6:15 PM UTC · Completed 6:34 PM UTC Commit: |
Retro Analysis: PR #5944Workflow type: Human-authored fork PR with agent-assisted manual review. No automated fullsend review/code/fix agents dispatched (fork PR security gate); only retro dispatched on merge. Timeline
Key ObservationsReview quality was exceptional. 0% false positive rate across 20 findings. Every finding was accepted and resulted in a code change. The reviewer traced through vendored OTel SDK source, wrote standalone test programs, and fuzzed the colon-break fix with 20,000+ adversarial inputs. Review ratchet consumed 40% of findings. 8 of 20 findings targeted code written to fix earlier findings, creating a 5-deep causal chain (UTF-8 repair → RecordError asymmetry → unbounded exception events → span limit semantics → env-var precedence). This chain consumed 5 of the PR's 7 calendar days. 29 wasted CI routing runs. All Evidence for existing issues
Proposals filed
|
2 participants
Summary
runErr == nil, and the agent span ended before the Agent API errors silently swallowed when Claude Code exits 0 with is_error:true #2786is_errortranscript check could flip the outcome. Any consumer keying on span/trace status (MLflow trace state, dashboards, filters) counted failed iterations as successes — one leg of the file-vs-MLflow discrepancy gating the tracing rollout.finalizeAgentSpan: a runtime error, a transcript-reported error, or a non-zero exit is Status Error, withRecordErrorattaching the exception event on both error paths (runtime error and transcript error), message UTF-8-repaired. The root span keys on the run outcome: validation passed is Ok (validation, not the last agent exit, is the success gate); otherwise a non-zerotelemetryExitCodeis Error even whenrunErris nil (harnesses without avalidation_loop).exit_codestays the raw process exit —fullsend.transcript_errormarks the Agent API errors silently swallowed when Claude Code exits 0 with is_error:true #2786 override, status is the failure signal, and the root span'sexit_coderemains the effectivetelemetryExitCode, so run-level filtering is unaffected; (2) status descriptions are bounded at 2,000 bytes total — prefix and truncation ellipsis included — on a UTF-8 rune boundary. No OTel, collector, or backend limit mandates a value; 2,000 reusesmaxTranscriptErrorLength, the repo's bound for the same class of error text (raised from this PR's earlier 256 after review). The transcript payload is budgeted for itstranscript error:prefix: messages within the 1,982-byte headroom pass through untouched, while parser-truncated messages (which arrive at 2,012+ bytes) are re-truncated, with the full text preserved on the exception event. The same bounding applies torunErrmessages and to the sandbox-create status (which embeds raw container logs — previously exported unbounded and unrepaired), because an invalid-UTF-8 or oversized status fails proto marshaling of the whole export batch. Exception events are bounded too — 8,192 bytes, UTF-8-repaired, cut on a rune boundary (added after review: the sandbox-create error embeds supervisor/gateway logs collected with no line limit, and the SDK never truncates event attribute values). The event still carries the fuller copy of every truncated status, and a parser-truncated transcript message stays whole on it even after sanitization growth. The transcript console line prints the same bounded, sanitized string the span records, through the rendering the GHA annotation path uses (TranscriptError.DisplayMessage), whose subtype fallback is bounded with the sametruncateErrortreatmentErrorMessagegets at parse time (the parser never truncatesSubtype). Span attribute values get a provider-level SDK limit of 8,192 characters — the SDK counts characters, not bytes, so multibyte content can reach four bytes per character on the wire, and it repairs invalid UTF-8 only when it truncates, so every dynamic string attribute is UTF-8-repaired at the source (stringAttr), with both SDK properties pinned by a canary test — the first non-empty ofOTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT/OTEL_ATTRIBUTE_VALUE_LENGTH_LIMITdecides alone, matching the SDK's own resolution: a parseable value there (including -1, unlimited) is honored as-is, and an override the SDK discards falls back to the 8,192 default — so free-text attributes — a transcript-derived model name, a pre-script skip reason — are bounded without per-site caps; the SDK applies this limit to span attributes only, which is why events are bounded at their call site instead. Classification (ADR 0050): error text on statuses and exception events — including the container/supervisor log excerpt a failed sandbox create embeds — is Level 1/2 "errors" metadata, not Level 3 content; the same excerpt was exported unbounded on the sandbox status before this PR, and both channels are now bounded. The ADR's Level 1 wording states this explicitly.Relates to #5361 (fixes the span-status leg only — the enrichment and cost-fidelity legs remain open, so this deliberately does not close it). The diff does not touch the root-span attribute block that #5788 edits.
Evidence (live runs,
minimal-explore, local sandbox)invalid_grant)is_error=true, exit 1, $0exit_code=1"agent exited with code 1",exit_code=1exit_code=0exit_code=0(unchanged)Root span on the failed run: Status ERROR
"validation failed after 2 iteration(s)"with anexceptionevent.Test plan
TestAgentSpanStatus/TestRootSpanStatus— table tests pinning both outcome→status mappings, including the Agent API errors silently swallowed when Claude Code exits 0 with is_error:true #2786 exit-0 override, the no-validation-loop root case, and validation-passed-with-non-zero-last-exit staying OkTestFinalizeAgentSpan—tracetest.SpanRecordertests pinning the exported span: status,fullsend.transcript_errormarker, rawexit_code, exception event, ended exactly onceTestTruncateStatusMsg/TestAgentSpanStatus_TranscriptBoundary— cap includes the ellipsis; multi-byte rune straddling the cap stays valid UTF-8; invalid UTF-8 repaired at any length; prefixed transcript status never exceeds the cap and a fitting message is not re-truncatedTestSetup_SpanAttributeValueLengthLimit/TestSpanLimits— a 100KB span attribute exports truncated to 8,192; operator env setting wins over the defaultTestRecordSanitizedError/TestTranscriptErrorMessage/TestTranscriptError_DisplayMessage— exception events bounded (ellipsis included, rune boundary, UTF-8 repaired); the transcript message is sanitized, bounded against an untruncatedSubtype, and worst-case sanitization growth (4,014 bytes at the colon-run fixed point) stays whole on the eventgo test ./internal/cli/ -racefull suite passes (183s)invalid_grant) exports agent spans ERROR + root ERROR with exception event; green control run unchanged, all Ok🤖 Generated with Claude Code