feat(telemetry): capture tool results in Level 3 content and emit execute_tool spans - #6603
feat(telemetry): capture tool results in Level 3 content and emit execute_tool spans#6603dhshah13 wants to merge 22 commits into
Conversation
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
PR Summary by QodoCapture correlated tool results in Level 3 telemetry
AI Description
Diagram
High-Level Assessment
Files changed (11)
|
Site previewPreview: https://0e7fb2b2-site.fullsend-ai.workers.dev Commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass on the Level 3 tool-result capture. Six MEDIUM findings, five as inline comments below; the sixth has no line inside the diff, so it is here.
MEDIUM — tool responses are placed in a role: "assistant" output message, deviating from the convention's own role placement
internal/cli/content_collector.go:340
Verified against the upstream source at semconv v1.37.0. Every part, including the new tool_call_response, is emitted inside a single contentMessage{Role: "assistant"} (content_collector.go:340). In docs/gen-ai/gen-ai-spans.md at v1.37.0, gen_ai.output.messages is defined as "Messages returned by the model", and the convention's worked example places tool_call_response parts under a separate "role": "tool" message inside gen_ai.input.messages — both confirmed in the fetched file. Client-executed tool results (which all Claude Code tools are) are not model output.
The choice is schema-valid: OutputMessage.parts admits ToolCallResponsePart, and its description covers "a built-in tool call outcome". But the PR body's claim that the mapping was "verified against semconv v1.37.0" covers only the field name (required: ["type","response"], which is correct) — the role placement was not checked, and it does deviate.
Suggestion: either split tool responses into their own {"role":"tool","parts":[...],"finish_reason":...} message in the array (the schema permits multiple messages, and the convention's example does exactly this), or record in the code comment / ADR that the single-assistant-message shaping is a deliberate deviation and why. Narrow the "verified against semconv v1.37.0" claim so it does not read as covering the whole mapping.
| // userContentItem is one content block within a user message. Only | ||
| // tool_result blocks are consumed; the block's content arrives either as | ||
| // a plain string or as an array of text blocks. | ||
| type userContentItem struct { |
There was a problem hiding this comment.
MEDIUM — is_error on tool_result is dropped, so failed tool calls are indistinguishable from successful ones
userContentItem (lines 85-89) decodes only type, tool_use_id and content. Anthropic's tool_result block also carries is_error, which Claude Code sets on failed and permission-denied tool calls, and ToolResultEvent (internal/runtime/event.go:54-57) has no field for it. A Level 3 consumer scoring agent behaviour therefore cannot tell a tool that errored from one that succeeded — the highest-signal distinction in a tool result — and must resort to text sniffing.
This is asymmetric with the other runtime in the same package: pi's piToolExecutionEndEvent (pi_progress.go:87-93) already decodes IsError bool and branches on it at line 520. Adding the field later is a change to a normalized-contract type; adding it now, while ToolResultEvent is brand new with one producer and one consumer, is free. Checked against semconv v1.37.0: ToolCallResponsePart has "additionalProperties": true, so a sibling key is schema-legal.
Suggestion: add IsError bool to ToolResultEvent, decode is_error on userContentItem, and surface it on the emitted part as a sibling key alongside response (the same latitude already used for summary on tool_call).
| } | ||
| } | ||
|
|
||
| case "user": |
There was a problem hiding this comment.
MEDIUM — the 1 MiB NDJSON line cap silently discards the largest tool results before the 8 KiB cap ever runs
streamBufSize = 1024 * 1024 (internal/runtime/event.go:5) bounds the bufio.Reader, and parseClaudeStream drains and skips any over-length line via the isPrefix loop at lines 158-163 with no event and no marker. That is pre-existing, but this new case "user" makes it load-bearing: the biggest tool results — exactly the ones maxToolResultBytes exists for — now vanish before the collector sees them, while the corresponding tool_call part still lands, producing a call with no response and no fullsend.content.truncated / dropped_bytes to explain the gap.
It also means the measured basis for the 8 KiB cap (derived from transcripts, not from what the parser actually ingests) is an upper bound on what production would capture. Both user-facing docs (docs/guides/dev/tracing.md:158-161 and docs/guides/infrastructure/distributed-tracing.md:101-105) describe only the 8 KiB per-result cap and the 256 KiB suffix, so a reader would not know a third truncation boundary exists.
Suggestion: document the 1 MiB NDJSON line ceiling next to the 8 KiB per-result cap so both truncation layers are visible, and/or emit a zero-length ToolResultEvent (or a debug note) when a line is skipped for length, so an over-buffer result surfaces as truncated rather than absent.
There was a problem hiding this comment.
Docs surfaced in 97eaf7f (both guides name the 1 MiB boundary next to the caps). Emit-on-skip: Deferred.
| c.parts = append(c.parts, p) | ||
| c.total += partSize(p) | ||
| c.evictOverflow() | ||
| case agentruntime.ToolResultEvent: |
There was a problem hiding this comment.
MEDIUM — a capped tool response is indistinguishable from a complete one; no per-part truncation marker
At lines 193-207, when a result exceeds maxToolResultBytes the head is discarded and the tail kept, but the emitted part is byte-identical in shape to an untouched one — same type, same id, a response string with no marker. The only signal is span-level fullsend.content.truncated / fullsend.content.dropped_bytes, which say something was cut but never which part; the same is true for the boundary tail-trim in Result (lines 320-327).
The PR's own measurements say the per-result cap fires on 11-22% of results in real runs, and tail-keeping removes precisely the identifying head (a Read result loses its file header and first lines; a Bash result loses the command echo and early output), so a scorer will read a fragment as a whole result. This also bears on the "Open to reviewer input" question in the PR body: tail-vs-head matters far less once the part says it was cut, and far more while it does not. ToolCallResponsePart has "additionalProperties": true in the v1.37.0 schema, so a marker key is schema-legal.
Suggestion: set a marker on the part when the per-result cap or the boundary trim fires (e.g. "fullsend.truncated": true), kept structural like id so it stays outside partSize and the exact-accounting invariant is untouched.
|
|
||
| // boundedID drops an id that exceeds maxToolIDBytes; the part survives | ||
| // without correlation rather than carrying a malformed identifier. | ||
| func boundedID(id string) string { |
There was a problem hiding this comment.
MEDIUM — the new id field bypasses the redaction pipeline the docs say every part goes through
This is distinct from the existing thread on this field ("Tool ids bypass budget", size accounting, marked fixed in 46a846f via boundedID) — this is the redaction path, which 46a846f did not touch.
At head, Content, Name, Summary and Response all pass through c.redact at assembly (Result, lines 293-296) and at eviction (evictOverflow, lines 240-243). ID is never redacted — not in Handle (lines 189/194), not in evictOverflow, not in Result. Meanwhile docs/guides/infrastructure/distributed-tracing.md:101 still reads "every part passes through security redaction (Unicode normalization, then secret masking) before reaching the span", which this PR makes false for the field it just added.
boundedID's own comment (lines 22-28) concedes ids arrive off the wire unbounded and untrusted enough to need a defensive length check, but stops at length: an id carrying invisible/bidi Unicode still lands verbatim on the span. Treating id as structural like type is sound only for a constant; id is stream-derived data.
Suggestion: run ID through c.redact alongside the other fields (findings counted), or — if keeping it out of both the size accounting and the scan is deliberate — amend the docs sentence at distributed-tracing.md:101 so it no longer claims coverage it does not have, and say in boundedID's comment why length is the only check applied to untrusted bytes.
| // encoding. A 255KB attribute was accepted whole by the pilot backend in | ||
| // live validation; larger is unproven, so the total stays put and tool | ||
| // results are bounded per part instead. | ||
| const maxContentBytes = 256 * 1024 |
There was a problem hiding this comment.
MEDIUM — "the total stays put" conflates a raw-byte budget with an encoded-size validation
This comment justifies 256 KiB with "A 255KB attribute was accepted whole by the pilot backend in live validation; larger is unproven" — but (a) 256*1024 = 262,144 raw bytes already exceeds the 255 KB figure cited, and (b) the budget is enforced on raw part bytes ("measured on the raw part bytes before JSON encoding", and partSize at line 108 sums raw len()), while the validated 255 KB was an encoded attribute value.
Result emits via json.Marshal (line 340), whose documented stdlib behaviour escapes <, > and & to 6 bytes each, doubles newlines/quotes/backslashes, and expands control bytes (ANSI ESC to \u001b, 6x). Assistant prose is sparse in those characters; tool results (file reads of Go/TS/HTML, diffs, JSON dumps, colourised command output) are dense in them, so 256 KiB of raw tool-result bytes can plausibly encode to well over 400 KiB of attribute value. internal/telemetry/telemetry.go:107-118 deliberately removes the SDK attribute cap under Level 3 on the stated grounds that "the content collector's byte budget is the size bound" — so nothing bounds the value actually put on the span. The largest completed gated run reported in the PR body was a 124,746-byte attribute, well short of the limit, so this was never exercised.
Suggestion: either bound len(res.OutputMessages) after json.Marshal against a validated encoded cap (re-marshalling a shorter suffix if exceeded), or correct the comment and the PR rationale to state that the enforced budget is raw bytes and the encoded value is unbounded and unvalidated at this content mix. At minimum, publish the measured encoded/raw ratio from the tool-result-heavy review run.
There was a problem hiding this comment.
Comment corrected and the measured encoded/raw ratio published in 14ffb12. Encoded-size enforcement: Deferred.
|
Role shaping: Intentional — documented at contentMessage in 97eaf7f (parts keep stream order; the iteration has one meaningful finish_reason, which OutputMessage requires per message). PR-body verification claim narrowed to field name and required-ness. |
5e9beed to
cd2757d
Compare
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass on the Level 3 tool-result capture. Six findings, all inline below (1 HIGH, 5 MEDIUM).
| c.evictOverflow() | ||
| c.appendPart(contentPart{Type: "tool_call", ID: boundedID(e.ID), Name: e.Name, Summary: e.Summary}) | ||
| case agentruntime.ToolResultEvent: | ||
| p := contentPart{Type: "tool_call_response", ID: boundedID(e.ID), Response: e.Result, IsError: e.IsError} |
There was a problem hiding this comment.
HIGH — Raw tool stdout now reaches exported spans, but the redactor has no pattern for the GCP/WIF bearer tokens this project runs on
The new case agentruntime.ToolResultEvent at content_collector.go:219 is what routes verbatim tool output (file contents, command stdout) onto gen_ai.output.messages and out over OTLP. Before this PR only the tool name plus an extractSafeContext summary was captured, so this exposure is newly reachable through this diff. The only filter is security.OutputPipeline() (UnicodeNormalizer + SecretRedactor), applied at Handle:224 for over-cap responses or at Result:337 otherwise.
I read the full pattern set at head. defaultPrefixPatterns (internal/security/redactor.go:129-155) covers openai/anthropic/github/slack/aws/stripe/sendgrid/hf/npm/pypi/gitlab/vault/age prefixes plus AIza Google API keys. defaultStructuralPatterns (:165-175) covers env-assignment, JSON-field, auth_header, private-key, and DB-URL forms. There is no pattern for Google OAuth access tokens (ya29.…), for bare JWTs (eyJ…), or for GCP STS/WIF token responses — and every structural pattern requires surrounding context a bare token lacks (a header name, an env var name, a JSON key, a URL scheme). docs/runtimes.md:60 states both runtimes run "on the same WIF credentials", so a gcloud auth print-access-token or a curl body printed by a Bash tool emits a live, bare, unlabelled bearer token straight into a span attribute that ships to run-telemetry.jsonl and the OTLP endpoint.
Note for triage: internal/security/redactor.go is NOT in this diff — the pattern list is pre-existing. The diff is what makes the gap load-bearing, so this is not out of scope. The user guide's warning covers only "proprietary code or PII" (docs/guides/user/how-to-emit-traces.md:118-119), which does not cover live credentials, while distributed-tracing.md:101 asserts "every part passes through security redaction" — true, but it implies coverage the pattern list does not have.
Suggested fix: Add prefix patterns for ya29\.[A-Za-z0-9._\-]{20,} and a JWT shape (eyJ[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}) to defaultPrefixPatterns, with a red-first test that a bare ya29. token in a tool result does not survive Result(). If that belongs in a separate PR, state the gap explicitly in the Level 3 docs next to the PII warning ("the redactor covers a fixed prefix list; bare OAuth/JWT bearer tokens are not matched") rather than leaving "every part passes through security redaction" to imply full coverage.
There was a problem hiding this comment.
Fixed in f49f8bc — both suggested patterns, with the prescribed collector-level test.
| // empty result produces no part) yet accumulate unboundedly, invisible | ||
| // to the size-based eviction. | ||
| func (c *contentCollector) appendPart(p contentPart) { | ||
| if contentBytes(p) == 0 { |
There was a problem hiding this comment.
MEDIUM — Empty, image-only, and is_error tool results vanish entirely — no part, no marker, no dropped-byte accounting
toolResultText (internal/runtime/claude_progress.go:388-408, confirmed at head) returns "" for any tool_result whose content array holds no text blocks — image blocks from Read on a PNG/JPEG, screenshots, documents — and for content that is neither a string nor a block array. appendPart (content_collector.go:238-241) then refuses the part because contentBytes(p) == 0, and contentBytes (:118-120) deliberately sums only Content/Name/Summary/Response, excluding IsError. Result:339 drops the part again for the fully-redacted case.
Net effect: content that existed on the wire disappears with no tool_call_response part, no fullsend.truncated, no contribution to fullsend.content.dropped_bytes, and no fullsend.content.truncated on the span. The correlated tool_call part reads as an unanswered call, indistinguishable from a call whose result never came back.
The two cases worth leading with, because they are what is new: (1) a tool_result with is_error:true and empty content produces no part at all — silently defeating the IsError field the author just added in 97eaf7f in response to the earlier review thread, since contentBytes does not count it; (2) image-only results vanish rather than surfacing as empty-but-present. TestContentCollector_EmptyToolResultProducesNoPart locks the drop-on-empty rule in without covering either case. This is also precisely the failure mode fullsend.truncated was added in this PR to prevent, and unlike the 1 MiB stream-line cap it is not listed under "Known limitations" in either guide.
Suggested fix: Let IsError keep an errored empty result alive by counting it as content-bearing in contentBytes (or special-casing it in appendPart), and emit a minimal part for non-text content (e.g. {type:"tool_call_response", id, response:"<non-text content omitted>", fullsend.truncated:true}) so id correlation survives. If dropping is intended instead, document it alongside the 1 MiB limitation in docs/guides/infrastructure/distributed-tracing.md and docs/guides/dev/tracing.md so the "absent rather than truncated" set is complete. Add a red-first test for the is_error:true + empty-content case either way.
There was a problem hiding this comment.
Fixed in ab5737e via the contentBytes route. Non-text-only results: documented absent — the placeholder variant would fabricate text into a content field. Red-first tests for both cases.
| kept := tailToRuneBoundary(p.Response, maxToolResultBytes) | ||
| c.evicted += len(p.Response) - len(kept) | ||
| p.Response = kept | ||
| p.Truncated = true |
There was a problem hiding this comment.
MEDIUM — Cap path marks a part fullsend.truncated even when redaction shrank it under the cap and nothing was cut
Distinct from the resolved thread at :218 (which asked for a per-part truncation marker to exist, fixed in 97eaf7f) — this is that same marker over-firing.
In Handle, p.Truncated = true at line 228 is unconditional inside the len(p.Response) > maxToolResultBytes branch (:220). Redaction runs first at :224 and can shrink the response — mask() (internal/security/redactor.go:121-126) collapses any value of 10+ chars to value[:4] + "..." = 7 bytes, and private_key replaces whole blocks — so a response that was, say, 8,220 bytes with one masked token becomes 8,187. tailToRuneBoundary then returns it whole (len(s) <= n at :436-438), c.evicted += 0 at :226, and yet the part ships with "fullsend.truncated":true.
If that is the only budget event in the iteration, res.Truncated = c.evicted > 0 (:325) is false, so the span carries a part flagged as a fragment while the span itself says nothing was truncated and fullsend.content.dropped_bytes is absent — a direct contradiction for a scorer. The pre-trim path at :294 already guards this correctly with if len(kept) < len(*bulk); the cap path is missing the same guard. Existing tests use clean strings, so redaction never shrinks anything and the case never fires.
Suggested fix: Mirror the pre-trim guard: after assigning kept, use if len(kept) < len(p.Response) { p.Truncated = true }, comparing against the post-redaction length since that is what the cut operates on. Add a test with a secret-bearing response just over maxToolResultBytes that redacts under it, asserting res.Truncated == false and no fullsend.truncated on the part.
There was a problem hiding this comment.
Fixed in ab5737e — the suggested guard and test.
| // Redact before the cap cut — the same invariant as every | ||
| // other cut: trimming raw bytes first could split a secret at | ||
| // the boundary past recognition. | ||
| p.Response = c.redact(p.Response, &c.findings) |
There was a problem hiding this comment.
MEDIUM — Capped tool results are redacted twice, double-counting fullsend.content.redactions for one secret
When a response exceeds maxToolResultBytes, Handle redacts it into c.findings (:224) and stores the sanitized text; Result then redacts the same stored text again into res.Findings (:337), which already contains a copy of c.findings (:329). This is not a hypothetical idempotence concern — I traced it against the actual mask() and pattern sources at head.
Most patterns are idempotent because mask() returns at most 7 bytes while the patterns require longer values, but db_connection_password is not: (?:postgres(?:ql)?|mysql|mongodb|redis)://[^:]+:([^\s"'}\]),;]{4,})@[^@\s/]+ (internal/security/redactor.go:174) needs only 4+ chars. Trace: postgres://user:supersecret@host → first scan captures supersecret (11 chars) → mask → supe... → text becomes postgres://user:supe...@host → second scan re-matches, because supe... is 7 chars and none of them are in the excluded class → a SECOND finding for the same secret, remasked to ***.
On the PR's own measurements 11-22% of results exceed the cap, so this is a routinely-taken path. Both fullsend.content.redactions and the Content capture redacted N finding(s) stderr warning overstate. The pre-existing pre-trim path had the same shape but only fired for a single >512 KiB part; this PR makes the double scan common.
Suggested fix: Redact once. Cheapest correct option: track a redacted bool on contentPart, set by the cap path, and have Result skip c.redact for Response on those parts. Alternatively cap on raw bytes and defer redaction entirely to Result, preserving the redact-before-cut invariant by redacting only the region around the cut.
There was a problem hiding this comment.
Fixed in ab5737e — the redacted-flag option, extended to the pre-trim and eviction paths. Coalescing into a pre-trimmed part clears the flag (a straddling secret needs the whole field visible), so the pre-trim rescan stays deliberately unconditional.
| | Roles | All | `review`/`retro` stay on Claude Code — they rely on sub-agent rosters | | ||
| | Effort | `--effort low..max` | `--thinking`, same levels (`high` when unset) | | ||
| | Security controls | Full matrix | Full matrix; stricter on failed-call sanitizing | | ||
| | Content capture (Level 3) | Text, reasoning, tool calls and tool results (correlating ids) | Same, minus tool results — pi's parser does not emit them yet | |
There was a problem hiding this comment.
MEDIUM — docs overstate pi's Level 3 parity — pi emits neither tool results nor correlating ids
Two user-facing docs claim Level 3 coverage pi does not have. Verified against parsePiStream at head: internal/runtime/pi_progress.go:529 emits ToolUseEvent{Name: evt.ToolName, Summary: summary} on tool_execution_end with no ID and no ToolResultEvent at all — even though piToolExecutionStartEvent.ToolCallID and piToolExecutionEndEvent.ToolCallID/Result/IsError are already decoded at :82-92 and simply never forwarded.
- docs/runtimes.md:58 reads
Text, reasoning, tool calls and tool results (correlating ids) | Same, minus tool results — pi's parser does not emit them yet. "Same, minus tool results" resolves to "text, reasoning, tool calls (correlating ids)" for pi, which is wrong — under pi everytool_callpart omits theidkey entirely. - docs/guides/user/how-to-emit-traces.md:116 tells the user the variable adds "text, reasoning, tool calls, and tool results to each
agentspan" with no runtime qualification, so a pi user enabling the gate gets no tool results at all.
The correct pattern already exists in this PR: docs/guides/infrastructure/distributed-tracing.md:93-95 qualifies ids with "when the runtime's stream provides one (Claude runs do)". The fix is making the other two files match it.
Suggested fix: Change the pi cell in runtimes.md to something like Text, reasoning, tool calls (no correlating ids) — pi's parser emits neither ids nor tool results yet, and add the same "when the runtime's stream provides them" qualification to how-to-emit-traces.md:116. Track the pi wiring as an explicit follow-up and note it is two changes, not one: pass ToolCallID into ToolUseEvent at pi_progress.go:529, then emit ToolResultEvent from tool_execution_end.
There was a problem hiding this comment.
Fixed in ab5737e — both files use the suggested wording; the two-change pi follow-up is noted in the PR body.
| const maxToolIDBytes = 256 | ||
|
|
||
| // maxToolResultBytes bounds one tool result's response within the | ||
| // suffix budget. Measured on three real review-agent runs (2026-08-25, |
There was a problem hiding this comment.
MEDIUM — The 8 KiB cap's stated measurement basis contradicts the capture scope this PR's own docs claim
The maxToolResultBytes docstring (:32-43) states the cap was "Measured on three real review-agent runs (2026-08-25, main thread)", and the PR body repeats "main-thread transcripts". But the collector is not main-thread-scoped, and this PR's own documentation says so: docs/guides/infrastructure/distributed-tracing.md:87-89 states captured tool results include "any sub-agent activity, unattributed". The new case "user" branch at internal/runtime/claude_progress.go:362-382 consumes every type:"user" line's tool_result blocks without filtering on thread origin, so sub-agent results become tool_call_response parts on the same span.
That internal contradiction is the load-bearing point. It matters because docs/runtimes.md:56 says the review/retro roles "rely on sub-agent rosters" — i.e. the exact roles used to derive the number are the ones whose real per-iteration volume a main-thread-only sample under-counts. The 222-389 KB totals, the p50/p90 figures, and the "78-89% of results untouched" claim therefore describe a strictly smaller population than production, and parent_tool_use_id is dropped (declared out of scope), so nothing in the output lets a consumer separate the two populations after the fact. (Supporting color, not verified from here: the Claude Agent SDK's forwardSubagentText option documents that tool_use/tool_result blocks from subagents are emitted by default.)
Suggested fix: Either re-derive the distribution from transcripts counting every user line rather than main-thread-only, or correct the docstring and PR body to say the basis is main-thread-only and that iterations with sub-agent rosters carry more results than measured, making the eviction-pressure claim a lower bound. Keeping the 8 KiB value is fine; stating the basis accurately is what matters, since it is the sole justification for the constant.
There was a problem hiding this comment.
Fixed in ab5737e — basis stated as main-thread lower bound; the gated review run corroborates the band including whatever sub-agent activity the stream carries.
ab5737e to
1d6cd6f
Compare
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass on the Level 3 tool-result capture at head (1d6cd6f). Two MEDIUM findings, both inline below.
| } | ||
| var texts []string | ||
| for _, b := range blocks { | ||
| if b.Type == "text" { |
There was a problem hiding this comment.
MEDIUM — Mixed text/non-text tool_result yields a silently partial, unmarked response
Verified at head (1d6cd6f). toolResultText joins only blocks whose type == "text" and silently discards every other block in the array. When a tool_result carries a MIX of text and non-text blocks, the emitted ToolResultEvent.Result holds only the text fragments, the collector builds a tool_call_response part from it, and nothing marks the loss — no fullsend.truncated on the part, no contribution to fullsend.content.dropped_bytes, no fullsend.content.truncated on the span. The span therefore carries a coherent-looking but incomplete response.
The PR's own test locks this in: TestParseClaudeStreamToolResultArrayContent (internal/runtime/claude_progress_test.go, confirmed at head) feeds [text "first block", image, text "second block"] and asserts Result == "first block\nsecond block", with the comment "expected text blocks joined by newline with image skipped".
This contradicts the invariant the PR states as its own design rule — "every cut part is marked fullsend.truncated so fragments never read as whole results" — and it is not covered by the documented escape hatch. docs/guides/infrastructure/distributed-tracing.md (head) enumerates exactly two absent-rather-than-truncated cases: stream lines beyond 1 MiB, and "results whose content is entirely non-text (for example images) produce no part". The mixed case is neither absent nor marked.
Not a duplicate of the existing thread at content_collector.go:279 ("Empty, image-only, and is_error tool results vanish entirely"): the reply there scoped the resolution to "Non-text-only results: documented absent", which is the all-or-nothing case. The partial case is untouched by that fix and by the docs sentence it produced.
Suggestion: Make the loss visible rather than silent. Cheapest correct option: have toolResultText return (text string, lossy bool) — lossy true when any non-text block was skipped — plumb it through ToolResultEvent as a Partial bool (the type is brand new with one producer and one consumer, so widening it is still free), and set contentPart.Truncated in Handle when it is true. That reuses the fullsend.truncated marker this PR already added and needs no new attribute. If plumbing a new event field is judged too heavy for this PR, at minimum widen the distributed-tracing.md sentence to name the mixed case explicitly and add a parser test asserting the documented behaviour, so the gap becomes a recorded decision rather than an unstated one.
There was a problem hiding this comment.
Fixed in 02e87bd — the (text, lossy) option as suggested: ToolResultEvent carries Partial, the part reuses fullsend.truncated, and the span-level fullsend.content.truncated fires too so affected spans stay filterable. An absent content key is not partial.
| {"age_secret_key", `AGE-SECRET-KEY-[A-Z0-9]{59}`}, | ||
| // Bare three-segment JWTs (and OIDC/WIF STS tokens) carry no | ||
| // surrounding context for the structural patterns to anchor on. | ||
| {"jwt", `eyJ[a-zA-Z0-9_-]{10,}\.[a-zA-Z0-9_-]{10,}\.[a-zA-Z0-9_-]{10,}`}, |
There was a problem hiding this comment.
MEDIUM — Shared secret-redactor patterns changed as a telemetry rider, affecting forge comments and console output
Verified at head (1d6cd6f). The new jwt (eyJ[a-zA-Z0-9_-]{10,}\.[a-zA-Z0-9_-]{10,}\.[a-zA-Z0-9_-]{10,}) and google_oauth_token (ya29\.[a-zA-Z0-9._\-]{20,}) entries were added to defaultPrefixPatterns() — i.e. into every NewSecretRedactor(), not to anything scoped to Level 3 content capture. Every prefix-pattern hit is emitted with Severity: "critical".
Consumers confirmed by code search at head, all reached through NewSecretRedactor() / OutputPipeline():
internal/cli/postreview.go—sanitizeReviewResultmasks review bodies and comments before they are posted to the forge.internal/cli/run.go— sanitizes the validation-feedback prompt injected into retry iterations.internal/runtime/claude_progress.go—progressRedactorfor console/CI display.internal/cli/content_collector.go— this PR's actual target.
The false-positive class is concrete and self-demonstrating: this PR's own internal/security/scanner_test.go adds a literal three-segment JWT fixture. A review agent quoting a JWT-shaped fixture, a docs example, or a decoded-token walkthrough from a target repo will now have that text masked to eyJh... inside the review comment posted to the PR.
Separately, ya29\.[a-zA-Z0-9._\-]{20,} places . inside the character class, so a match runs greedily through following sentence punctuation and adjacent words until whitespace — over-masking surrounding prose when a token appears mid-sentence. Note the adjacent google_api_key pattern (AIza[a-zA-Z0-9_-]{35}) deliberately excludes ..
This is genuinely new: internal/security/redactor.go carries no review thread, and the thread that requested these patterns (content_collector.go:255, "the redactor has no pattern for the GCP/WIF bearer tokens") was about their ABSENCE, not about the scope or shape of what landed.
Scope note, checked and worth stating: the run-blocking paths (scanRepoContextFiles, scanAgentFile/scanSkillDir/scanPluginDir) use security.InputPipeline(), not the secret redactor, so these patterns cannot hard-fail an agent run. The impact is masked forge output and display noise, not a broken run.
Suggestion: First, narrow the regex: drop . from the ya29 character class (ya29\.[a-zA-Z0-9_\-]{20,}) so a mid-sentence token stops at the token rather than running to the next whitespace. That is a one-character fix with no downside.
Then make the blast radius a stated decision rather than a side effect. Say in the PR description that this changes forge-comment sanitization and console display repo-wide, not just span content. If you want to keep the change tightly scoped to what the telemetry requirement actually needs, gate the jwt pattern behind a redactor option that only the content collector enables — sanitizeReviewResult posting to a PR has a very different false-positive cost than a span attribute. Otherwise, evidence the FP rate: run the two regexes over a corpus of recent review bodies and report the hit count.
There was a problem hiding this comment.
Fixed in 5a11ea3 — dot dropped from the ya29 class per the suggestion, plus a literal c. alternative so service-account tokens (the WIF shape) still match. Blast radius now stated in the PR body as a decision, with the evidence run: both patterns hit zero times over 2.1MB of this repo's recent review and issue comment bodies. Gating not taken — a real JWT in a forge comment should mask, and the measured FP cost is zero.
afaf16d to
02e87bd
Compare
| // the match through punctuation into adjacent prose); the literal | ||
| // c. alternative covers service-account tokens, whose 1-char | ||
| // first segment would otherwise defeat the {20,} quantifier. | ||
| {"google_oauth_token", `ya29\.(?:c\.)?[a-zA-Z0-9_\-]{20,}`}, |
There was a problem hiding this comment.
MEDIUM — New ya29.c. / JWT redactor patterns are telemetry-only — the PostToolUse leak-prevention hook stays blind to both
Verified at head (02e87bd) by reading both files and executing the regexes.
This PR teaches the Go redactor two credential shapes it did not know before:
// redactor.go:150 — the c. alternative was added specifically so
// WIF/service-account tokens match
{"google_oauth_token", `ya29\.(?:c\.)?[a-zA-Z0-9_\-]{20,}`},
// redactor.go:163
{"jwt", `eyJ[a-zA-Z0-9_-]{10,}\.[a-zA-Z0-9_-]{10,}\.[a-zA-Z0-9_-]{10,}`},The sibling layer whose documented job is to stop those same secrets before the model sees them has neither. internal/security/hooks/secret_redact_posttool.py (module docstring: "Intercepts tool results (Bash, WebFetch, Read) and redacts secrets before they enter the LLM context window") carries at line 41:
("google_oauth_token", re.compile(r"ya29\.[A-Za-z0-9_-]{30,}"))That pattern cannot match a ya29.c.<blob> token: the literal . after c is outside the character class, so the {30,} run breaks after one character. Executed directly:
py ya29\.[A-Za-z0-9_-]{30,} on 'ya29.c.' + 'A'*80 -> no match
go ya29\.(?:c\.)?[a-zA-Z0-9_-]{20,} on the same input -> match
The hook also has no eyJ / three-segment-JWT pattern at all — grep for eyJ over the whole file returns nothing, and its structural patterns (env_secret, json_secret, auth_header) all require surrounding context a bare token lacks, exactly as the earlier reviewer argued for the Go side at content_collector.go:255. _KNOWN_PREFIX_RE (line 224) lists ya29\. only as a fixture-detection prefix, not as a match pattern.
Net effect: the repo now demonstrably knows the ya29.c. and bare-JWT shapes exist and are live credentials on the WIF setup both runtimes run on, but guards them only downstream in span content. A gcloud auth print-access-token or an STS/WIF token response printed by a Bash tool on a successful call still reaches the model context unmasked by the sandbox hook.
Scope note for triage: secret_redact_posttool.py is not in this diff and the gap is pre-existing — this PR reveals the drift rather than creating it. It is anchored at redactor.go:150 because that is the in-diff line that establishes the shape.
Novelty checked, not assumed: the two existing redactor-adjacent threads are content_collector.go:255 (the absence of Go patterns; "Fixed in f49f8bc") and redactor.go:163 (repo-wide blast radius of the shared redactor; "Fixed in 5a11ea3"). Grepping the full set of posted review comments for posttool / secret_redact / hooks/ returns 0 hits — no existing thread mentions the Python hook.
Suggestion: Decide and record which layer owns these shapes. If service-account/WIF ya29.c. tokens and bare JWTs are in scope for the PostToolUse hook's stated leak-prevention duty (not just span redaction), mirror the two patterns into _PREFIX_PATTERNS in internal/security/hooks/secret_redact_posttool.py — ya29\.(?:c\.)?[A-Za-z0-9_-]{20,} plus a JWT entry — with the hook's usual fixture test. If this PR is deliberately span-only and the hook's coverage is out of scope, say so in a comment next to the new google_oauth_token entry in redactor.go so the two inventories do not silently drift further; the hook already has a matching-but-weaker entry at line 41, which makes the divergence easy to mistake for parity.
There was a problem hiding this comment.
Fixed in 56d99e0 — mirror taken, not the scope-note: the hook's stated duty covers exactly this leak. All three shapes now match the Go side (google_oauth_token with the c. alternative, jwt, and github_server_token — pre-push verification found the hook's combined gh*_ pattern also stopped at the first dot of the JWT-wrapped installation format, leaving payload+signature clear, so that one is mirrored too). Hook fixture tests for all three; 392 hook tests green.
d277b25 to
56d99e0
Compare
|
On the examples, I see some summaries for tools being trimmed, can we get the full list of arguments ( |
|
Could you point me to the reason why we are using this way to add tool calls? Should we use normal spans for tools? I see https://github.com/open-telemetry/semantic-conventions-genai/blob/main/docs/gen-ai/gen-ai-agent-spans.md#execute-tool-span and https://github.com/open-telemetry/semantic-conventions-genai/blob/main/docs/gen-ai/gen-ai-spans.md#execute-tool-span |
|
The summary is the parser's bounded console context reused on the part (extractSafeContext — patterns display at 50 chars, which is the trim in that trace), and #6429 deliberately never fabricated an |
allow() charged fullsend.tool_spans.dropped on every event past the cap, so a call rejected at its tool_use event was charged again when its result arrived and found no open span: the attribute read 2N for N rejected calls, against its documented meaning. The cap test sent a bare result past the cap but never a result for a rejected call, so it could not see it. The overflow is now charged once, at the rejected tool_use event; past the cap a result with no open span is not charged at all, since telling a rejected call's result from an orphan would need a set of rejected ids of agent-controlled size. TestToolSpanTracker_DroppedCountsEachCallOnce sends complete use/result pairs past the cap and expects N; TestToolSpanTracker_OrphanResultsCountTowardTheCap pins that an unmatched result under the cap still spends a slot; the attribute's doc row says the same. Raised in review of fullsend-ai#6603. Signed-off-by: Dharit Shah <dhshah@redhat.com>
The hook's comments on the google_oauth_token and jwt prefix patterns claimed they mirror the Go redactor; main's redactor has neither shape. Both are added on the Go side by fullsend-ai#6603, which this PR was split from, so the comments now say that. The github_server_token comment stays: that pattern is on both sides today. Raised in review of fullsend-ai#7009. Signed-off-by: Dharit Shah <dhshah@redhat.com>
Claude Code's stream-json delivers tool results as tool_result content blocks inside user-type lines, which the parser previously dropped. Add a ToolResultEvent to the normalized event contract and emit it for each tool_result block, flattening string and text-block-array content. Also surface the tool_use block id on ToolUseEvent so tool calls and their results can be correlated downstream. Only the Claude runtime emits ToolResultEvent; the renderer ignores it by design — its consumer is the Level 3 content collector (ADR 0050), wired in a follow-up commit. Signed-off-by: Dharit Shah <dhshah@redhat.com>
Handle ToolResultEvent in the Level 3 content collector as the schema's
ToolCallResponsePart ({type:"tool_call_response",id,response} — the
required field is response, per semconv v1.37.0), and carry the new
ToolUseEvent.ID on tool_call parts so calls and results correlate.
Response bytes follow every existing invariant: redacted before any cut
(at assembly, at eviction, and in the over-double-budget pre-trim),
counted exactly in the dropped-byte accounting, tail-trimmed at the
suffix-budget boundary like text (the id survives the trim). An empty
result carries no content-bearing bytes and produces no part — so no
tool_call_response part ever omits its schema-required response key.
Part ids, like the type field, are structural rather than captured
content and stay outside the size accounting.
Signed-off-by: Dharit Shah <dhshah@redhat.com>
Measured on three real review-agent runs (main thread only): uncapped tool results total 222-389KB per iteration, overflowing the 256KiB content budget on two of three runs — the suffix budget would then evict whole older parts. An 8KiB per-result cap kept those runs at 127-255KB with 78-89% of results untouched (per-result p50 2-3.5KB, p90 9-19KB, max 78KB), lowering eviction pressure. Heavier iterations still overflow and evict oldest-first, marked exactly as ever; the cap value is revisitable when other roles' distributions are measured. The cap keeps each response's tail — the ordered-suffix policy extended to individual results; no consumer requirement has confirmed either direction yet. It follows the redaction-before-truncation invariant: the full response is scanned before the head cut, so a boundary-straddling secret is redacted while still recognizable. Capped bytes land in DroppedBytes and set the truncated marker. Signed-off-by: Dharit Shah <dhshah@redhat.com>
Flip the tool-results Planned callout to shipped in the tracing reference, note the correlating ids and the 8KiB per-result bound, add the Level 3 row to the claude/pi comparison, and extend the dev guide's collector walkthrough with the tool_call_response mapping. Signed-off-by: Dharit Shah <dhshah@redhat.com>
Cover the three defensive paths Codecov flagged: a user line whose message is not an object, a user message whose content is a plain string (a real wire shape, no tool_result blocks to extract), and a tool_result whose content is neither string nor block array (flattens to an empty result that still carries its id). Signed-off-by: Dharit Shah <dhshah@redhat.com>
Ids ride outside the content size accounting as structural bytes, but the stream decodes them unbounded and Level 3 lifts the SDK attribute cap — an oversized id would bypass every bound the collector enforces. Treat anything beyond 256 bytes (real ids run tens of bytes) as malformed and drop it at Handle; the part survives uncorrelated rather than carrying a truncated id that could falsely collide. Also add tool results to the Level 3 row of the tracing levels table, which the docs commit missed. Both raised by Qodo review on this PR. Signed-off-by: Dharit Shah <dhshah@redhat.com>
The final review gauntlet confirmed three mechanisms the per-id bound alone left open. Ids serialize into the attribute but counted toward nothing, so their bytes bypassed the budget in aggregate — partSize now includes them, making the dropped-byte accounting exact over every serialized part byte, and the suffix boundary reserves a part's id bytes before fitting its response tail. Ids were also the only stream-derived string never passed through the redaction pipeline — they are scanned now, and a finding drops the id entirely rather than substituting one that could falsely collide. Parts with no content-bearing bytes are refused at Handle: they contributed nothing to output yet accumulated unboundedly, invisible to size-based eviction. Also disclose two residuals instead of implying their absence: the marshaled attribute carries JSON syntax/escaping above the counted budget (pre-existing fullsend-ai#6429 semantics), and stream lines beyond 1MiB are skipped whole — newly lossy for tool results, noted at the skip site. Signed-off-by: Dharit Shah <dhshah@redhat.com>
… role shaping From waynesun09's review. Failed tool calls now carry the wire's is_error through ToolResultEvent onto the part as a sibling key — the highest-signal distinction in a result, added while the contract type has one producer and one consumer. Every part whose bulk field is cut (per-result cap, suffix boundary, pre-trim) is marked fullsend.truncated, so a consumer never reads a fragment as a whole result; both keys are fixed-size structural booleans outside the byte accounting, schema-legal via additionalProperties. Also document the single-assistant-message shaping as a deliberate, schema-valid deviation from the convention's role:tool example (stream order and the one-finish_reason-per-iteration semantics), and surface the parser's 1MiB stream-line ceiling in both guides as the boundary that precedes the 8KiB and 256KiB caps. Signed-off-by: Dharit Shah <dhshah@redhat.com>
When the suffix boundary's rune-boundary window lands entirely inside a trailing multi-byte rune, the tail is empty and the part drops whole — but only its bulk bytes were charged, undercounting DroppedBytes by exactly the id length. Charge the full part size on any whole drop, and align the consumer attribute table and a stale test comment with the ids-counted accounting. Signed-off-by: Dharit Shah <dhshah@redhat.com>
Level 3 tool-result capture routes verbatim command stdout onto exported spans, and the runs emitting it authenticate through WIF — yet the pattern set had no shape for the bare ya29. access tokens and three-segment JWTs those credentials appear as. Both carry no surrounding context for the structural patterns to anchor on. Raised by waynesun09's review of the capture PR. Signed-off-by: Dharit Shah <dhshah@redhat.com>
From waynesun09's second review pass, plus one regression the pre-push
gauntlet caught in these very fixes:
- An errored empty result is signal, not absence: is_error now counts a
fixed serialized footprint in contentBytes, so the part survives as
{type, id, is_error, response:""} — a custom marshaler guarantees
the schema-required response key on every response part. Non-text-only
results stay absent, now documented with the other absent case.
- Redaction shrink under the per-result cap no longer marks a part
truncated; the cap-path guard mirrors the pre-trim's.
- Capped results are scanned exactly once: the cap and pre-trim record
the scan, and Result and eviction skip those bytes — but bytes
coalesced into a pre-trimmed part clear the flag again, because a
straddling secret needs the whole field visible (the pre-push
gauntlet reproduced a raw token reaching the span without this).
The pre-trim rescan stays deliberately unconditional for the same
reason.
- Docs: pi emits neither ids nor tool results yet (matrix and user
guide corrected); the 8KiB cap's main-thread measurement basis is
stated as a lower bound on production volume.
Signed-off-by: Dharit Shah <dhshah@redhat.com>
The ya29 class no longer contains a dot — a mid-sentence token ran the match through punctuation into adjacent words — and gains a literal c. alternative: service-account access tokens (the shape WIF-provisioned runs mint) have a one-character first segment that would otherwise defeat the length quantifier and leak the token whole. Measured over 2.1MB of this repo's recent review and issue comment bodies, the new patterns hit zero times. Signed-off-by: Dharit Shah <dhshah@redhat.com>
From waynesun09's third review pass. A tool_result mixing text with non-text blocks kept only the text with nothing marking the loss. toolResultText now reports the skip, ToolResultEvent carries it as Partial, and the collector sets the part's fullsend.truncated — and surfaces it on the span-level marker too, the only cheap filter for affected spans; no byte count is fabricated for content the parser never measured. An absent content key carries nothing to skip and is not partial, keeping errored-empty parts unmarked like their explicit empty-content equivalents. Signed-off-by: Dharit Shah <dhshah@redhat.com>
The contentMessage comment settled only the role-placement half — part admission is schema-valid — while reading as settling the whole question. The registry note on gen_ai.output.messages separately ties each message to exactly one generation, which packing an iteration into one assistant message deviates from independently. Name both counts and the shared rationale; per-generation messages, if ever needed, are a deliberate carrier change. Raised by waynesun09's review. Signed-off-by: Dharit Shah <dhshah@redhat.com>
Review of the Level 3 tool-result capture asked why tool calls are parts of the message record rather than spans. Nothing had decided that: ADR 0050 never named the spans and left granularity to fullsend-ai#294, and at semconv v1.37.0 the execute_tool span is metadata-only, so the message record was the only conventional home for content. Both shapes fit together, and this adds the spans. toolSpanTracker (internal/cli/tool_spans.go) opens an `execute_tool <tool name>` span under the iteration's agent span when the runtime reports a call and ends it when the result arrives — runner-side receipt at both ends, one clock: tool_use lines carry no timestamp, tool_result lines carry a sandbox-clock one the parser ignores, and the start is arguments-complete rather than execution start. Attributes per v1.37.0: gen_ai.operation.name, gen_ai.tool.name (redacted, then bounded to 256 bytes), gen_ai.tool.call.id (ids beyond 256 bytes are dropped, never truncated); a result flagged is_error sets error.type=tool_error and status Error. A call still open when the iteration ends — the runtime was stopped, or its result line exceeded the parser's 1 MiB cap — is closed as error.type=unanswered; a result for a call never reported becomes a near-zero-duration span marked fullsend.tool.unmatched; events without an id produce no span: pi and codex streams, and server-side tools, whose result never arrives as a tool_result, so the parser now leaves their id empty. At most 1,024 spans are recorded per iteration — Finish ends every open call in a burst right before the agent span ends, and an agent-controlled flood would otherwise fill the OTLP batch queue and evict the agent span; the overflow is recorded as fullsend.tool_spans.dropped on that span. Tool content stays on gen_ai.output.messages, the scorer contract. The spans are Level 1 metadata, so RunParams.OnEvent is now always installed: iterationEventHandler calls the renderer first (console output unchanged), then the content collector, then the tracker. The nil-handler invariant from fullsend-ai#6429 no longer holds; its test is replaced. ADR 0102 records the topology, the runtime-native OpenTelemetry route it declines for now, and the sub-agent nesting it leaves deferred; it settles the granularity question in fullsend-ai#294 and scopes retention and access out. Docs: tracing reference, dev guide, user guide, runtimes matrix, architecture.md, the observability problem doc, an annotation on ADR 0050. Raised by rh-hemartin's review. Signed-off-by: Dharit Shah <dhshah@redhat.com>
The execute_tool span's gen_ai.tool.call.id went straight from boundedID to the attribute, bypassing the scan-then-bound rule this PR applies to the tool name and, in the collector, to the same id field. The spans are Level 1 metadata written to the telemetry file the post-run output scan exempts, so the exemption's own justification did not cover the id. safeID scans the id through the output pipeline and drops the attribute on any finding — never a substituted id, which could collide with another call's — while the raw id still keys use/result correlation; a test pins that keying with two ids whose secrets mask to the same token. The run.go exemption comment and both tracing guides now name ids alongside tool names. Raised by waynesun09's review. Signed-off-by: Dharit Shah <dhshah@redhat.com>
ADR 0102 said gen_ai.tool.call.arguments and .result exist only in the newer GenAI conventions repository. They are in the tagged conventions from v1.38.0 on (Opt-In, Development), and the otel module this repo depends on bundles that package; the ADR's option 4 now says so, and its decision names call ids alongside tool names as sanitized. Signed-off-by: Dharit Shah <dhshah@redhat.com>
Review asked for either a knob to throttle execute_tool emission or an explicit decision. ADR 0102 now says the volume is intended: the cap is a pathological-case backstop, not a knob, and no switch or sampler is added — OTel sampling is per trace and would drop whole runs rather than thin these spans, and a fullsend-specific switch would be a new configuration surface this series avoids. Raised by waynesun09's review. Signed-off-by: Dharit Shah <dhshah@redhat.com>
0102 is also claimed by fullsend-ai#6972 (generate custom agents from the CLI); 0101, 0103, 0105, 0106 and 0107 are claimed by other open PRs and 0104 and 0105 are on main, so 0108 is the lowest free number. Links in ADR 0050, architecture.md and operational-observability.md follow. Signed-off-by: Dharit Shah <dhshah@redhat.com>
allow() charged fullsend.tool_spans.dropped on every event past the cap, so a call rejected at its tool_use event was charged again when its result arrived and found no open span: the attribute read 2N for N rejected calls, against its documented meaning. The cap test sent a bare result past the cap but never a result for a rejected call, so it could not see it. The overflow is now charged once, at the rejected tool_use event; past the cap a result with no open span is not charged at all, since telling a rejected call's result from an orphan would need a set of rejected ids of agent-controlled size. TestToolSpanTracker_DroppedCountsEachCallOnce sends complete use/result pairs past the cap and expects N; TestToolSpanTracker_OrphanResultsCountTowardTheCap pins that an unmatched result under the cap still spends a slot; the attribute's doc row says the same. Raised in review of fullsend-ai#6603. Signed-off-by: Dharit Shah <dhshah@redhat.com>
3348bd8 to
c80a197
Compare
The hook's comments on the google_oauth_token and jwt prefix patterns claimed they mirror the Go redactor; main's redactor has neither shape. Both are added on the Go side by fullsend-ai#6603, which this PR was split from, so the comments now say that. The github_server_token comment stays: that pattern is on both sides today. Raised in review of fullsend-ai#7009. Signed-off-by: Dharit Shah <dhshah@redhat.com>
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass at head (c80a197). Two findings, both inline below (1 HIGH, 1 MEDIUM).
|
|
||
| if runErr != nil { | ||
| attachIterationContent("error") | ||
| recordToolSpanOverflow(agentSpan, toolSpans.Finish()) |
There was a problem hiding this comment.
HIGH — Cancellation path never finishes open execute_tool spans, so the documented error.type=unanswered never fires
Verified at head c80a197. runAgent creates the tracker at run.go:2232 and calls recordToolSpanOverflow(agentSpan, toolSpans.Finish()) on exactly two of the three finalization paths — the runErr != nil branch (run.go:2288, this hunk) and the success branch (run.go:2336, hunk @@ -2329,6 +2333,7 @@).
The third path, handleRunCancellation (called at run.go:2277, body at 3737-3764), calls only attachIterationContent("error") and finalizeAgentSpan(...) — which does span.End() on the agent span — and the caller then returns immediately via the cancelled branch. toolSpans.Finish() is never called, so t.open is dropped non-empty:
- every in-flight
execute_toolspan is neverEnd()ed, so it never reaches theSimpleSpanProcessorfile sink or the OTLPBatchSpanProcessor; fullsend.tool_spans.droppedis never set on the agent span, even when that iteration overflowed the 1,024-span cap.
docs/guides/dev/tracing.md:148 and ADR 0108 (docs/ADRs/0108-tool-call-span-topology.md:63-64) both document that a call which never gets a result because "the runtime was stopped" is closed by Finish() as error.type=unanswered. That documented behaviour does not fire on the stop path — which is the one path where it was meant to.
Scope note: ctx in runAgent is never deadline-bound (the only WithTimeout derivations in run.go are preflightCtx/flushCtx/etc.), so this is not the iteration-timeout path — the iteration budget is RunParams.Timeout, surfaces as an exit code, and flows through the success branch that does call Finish. The trigger is signal cancellation: SIGINT, and the GitHub Actions SIGTERM path that handleRunCancellation's own doc comment names (#6936).
The fix is not moot on the cancel path: because the file sink is a SimpleSpanProcessor that writes on OnEnd, ending the spans lands them in run-telemetry.jsonl even when SIGTERM kills the process before the OTLP flush at run.go:1588.
Suggestion — hoist the finish out of the branch: call recordToolSpanOverflow(agentSpan, toolSpans.Finish()) once immediately after rt.Run returns, before the three-way runErr/cancellation/success split, so all three paths share one finish (and drop the two now-duplicate calls at 2288 and 2336). Alternatively, pass the tracker into handleRunCancellation and finish before finalizeAgentSpan.
Coverage in internal/cli/telemetry_run_test.go: cancel the context mid-iteration with one open tool call, and assert an execute_tool line with error.type=unanswered and the agent span's spanId as parentSpanId lands in the file sink.
There was a problem hiding this comment.
Fixed in e0d350e. finalizeAgentSpan now finishes the tracker before ending the agent span, so the cancellation path shares it with the other two and the explicit calls are gone; a test cancels with an open call and an overflow and asserts the execute_tool span lands in the file sink as unanswered under the agent span with the overflow recorded.
|
|
||
| fullsend observes the runtime's stream rather than executing tools. The | ||
| normalized `ToolUseEvent`/`ToolResultEvent` pairs carry a call id, a tool | ||
| name and an `is_error` flag; `tool_use` lines carry no timestamp and |
There was a problem hiding this comment.
MEDIUM — the timing rationale rests on a wire-format premise that is false: tool_use lines do carry a timestamp
Verified at head c80a197. This Context paragraph (lines 35-36) states:
tool_uselines carry no timestamp andtool_resultlines carry a sandbox-clock one
The Decision (lines 58-60) then uses that asymmetry to justify runner-side receipt timing as "one clock", and the same sentence is the answer given to @rh-hemartin in the PR conversation after he wrote "I would try to refactor it to output normal tool spans if we can scrap timestamps."
The premise is false for the code path fullsend actually uses. internal/runtime/claude.go:354-356 invokes Claude Code with --print --verbose --output-format stream-json and no --include-partial-messages, so no stream_event lines occur and tool_use blocks arrive on assistant lines. A live probe (claude -p --verbose --output-format stream-json --allowedTools Read, Claude Code 2.1.267):
- the
assistantline carrying thetool_useblock has top-level keys[message, parent_tool_use_id, request_id, session_id, timestamp, type, uuid], withtimestamp='2026-09-10T18:45:57.443Z'; - the
userline carrying thetool_resulthas[message, parent_tool_use_id, session_id, timestamp, tool_use_result, type, uuid], withtimestamp='2026-09-10T18:45:58.740Z'.
Both envelopes come from the same serializer, and the ADR already concedes the user line carries one — so "no timestamp" on the assistant side cannot be right. Both ends of a call are therefore available on one clock (the sandbox's) and bracket execution more tightly than runner receipt does.
This is not the tool_spans.go:22 thread (closed "Intentional" and approved) — that one is about span volume and the absence of a throttle knob, not about timing provenance. The approval does not settle this, because the design answer that earned it rests on the false fact.
Suggestion — correct the premise here at line 35 and in the PR-conversation reply. Then either:
(a) keep runner-side receipt timing and state the real trade-off — no cross-host clock skew, at the cost of looser bracketing — recording it as a decision rather than deriving it from a wire-format claim; or
(b) parse the top-level timestamp on assistant/user lines into ToolUseEvent/ToolResultEvent and pass it via trace.WithTimestamp at start/end, which is what the "normal tool spans" ask was reaching for.
Either way, reconcile the tracker doc comment at internal/cli/tool_spans.go:42-47, which describes runner-side receipt without stating why.
There was a problem hiding this comment.
Fixed in e0d350e. The assistant envelope re-probed locally on 2.1.235 carries the timestamp too; ADR 0108, the tracker comment and the body now state receipt timing's real trade-off (the agent span's own clock, so no cross-host skew, and the one source every runtime provides, at the cost of bracketing that trails the sandbox by the pipe latency) and record decoding Claude Code's timestamps as open. My 2026-09-01 comment is corrected in the conversation.
…correct the timing premise Two findings from review of fullsend-ai#6603. runAgent finished the tool-span tracker on its error and success paths but not on the third: handleRunCancellation ended the agent span and the caller returned, so on SIGINT or the Actions SIGTERM the open execute_tool spans were never ended, never reached the file sink, and fullsend.tool_spans.dropped was never recorded — the path where error.type=unanswered was meant to fire. finalizeAgentSpan now finishes the tracker before ending the agent span, so every path that ends the agent span shares it, and the two explicit calls are gone. Tests cancel with one open call and an overflow and assert the execute_tool span lands in the file sink as unanswered under the agent span with the overflow recorded, and that finalizeAgentSpan ends the tool spans before their parent. ADR 0108 justified runner-side receipt timing by claiming tool_use lines carry no timestamp. They do: Claude Code's assistant (tool_use) and user (tool_result) stream lines each carry a sandbox-clock timestamp the parser does not decode — the review probed both on 2.1.267, and the assistant envelope was re-probed locally on 2.1.235 — while pi's tool-execution lines and codex's items carry none. The decision stands on its real trade-off — the parent span's clock, so no cross-host skew, and the one source every runtime provides, at the cost of bracketing that trails the sandbox by the pipe latency — and the ADR and tracker comment now say so; decoding Claude Code's timestamps is recorded as open. Raised in review of fullsend-ai#6603. Signed-off-by: Dharit Shah <dhshah@redhat.com>
…d tighten the span docs From a review-squad pass over the branch. The google_oauth_token pattern special-cased ya29.c. because a one-char first segment defeats the length floor; Google's workforce STS response carries ya29.dr., which defeated it the same way. The optional segment is now any one or two lowercase letters, and the same change is made to fullsend-ai#7009's hook pattern. Two parser edges gain tests: undecodable tool_result content reports Partial (the branch was unpinned), and a server_tool_use block on an assistant line — the path fullsend actually runs, since the launch never passes --include-partial-messages — produces no event of any kind while the tool_use beside it keeps its id. Comments say why the stream_event tool-id slot is single-slot and that the flat user-line shape is a defensive fallback rather than an observed version. Docs: fullsend.content.truncated also fires for a kept parser-side fragment; error.type=unanswered has a third cause (a call superseded by a second tool_use with the same id, ended at the reuse); server_tool_use blocks produce no event; the span cap assumes the default OTEL_BSP_MAX_QUEUE_SIZE and the unsourced call-count figure is replaced by the evidence run's; ADR 0108 says which pi lines the parser reads and records upstream's display-only caveat on the open timestamp item. Signed-off-by: Dharit Shah <dhshah@redhat.com>
… again From a review-squad pass over the branch. The google_oauth_token pattern special-cased ya29.c. because a one-char first segment defeats the length floor; Google's workforce STS response carries ya29.dr., which defeated it the same way. The optional segment is now any one or two lowercase letters, matching the same change on the Go side in fullsend-ai#6603; tests cover ya29.dr. and ya29.d. Since the resolved-target check landed, the tests for the '@', URL and '~' refusals passed for the wrong reason: their fixtures named entries that did not exist, so the target check refused first and the refusals were unpinned (two mutants survived). The raw twins now exist on disk inside the checkout, so only the refusal decides. The environment tests also pin the module sources: the hook's only environ read is the trace id and the chain's are the trace id and the canary token, so a boundary read under any name fails them, not only one guessed variable. The contract doc and the PR body now state the class the skip does not guard against — a file copied, hard-linked, moved or raced into the checkout — and the codex docstring says pi sends cwd on PostToolUse only. Signed-off-by: Dharit Shah <dhshah@redhat.com>
|
Correction to my comment of 2026-09-01: |
3 participants
Second PR in the ADR 0050 Level 3 series, following #6429 (which shipped the gate, collector, and budget for text/reasoning/tool calls). This adds the tool results those parts referenced — the "Next in this series" item from #6429's description — as one change: the parser extension ships with its consumer. Review then asked why tool calls are parts rather than spans, and nothing had decided that — so this PR also adds one
execute_toolchild span per tool call under eachagentspan (metadata only; content stays on the message record) and records the topology in ADR 0108.What this does
internal/runtime)ToolResultEvent{ID, Result}in the normalized contract, emitted from thetool_resultblocks in Claude stream-jsonuserlines (previously dropped). Handles both the nestedmessage.contentand older flat shapes, and both content forms (plain string; text-block arrays joined with newlines, non-text blocks skipped).ToolUseEventgainsIDfrom thetool_useblock so calls and results correlate.internal/cli)Handlecase maps it to the schema'sToolCallResponsePart—{type:"tool_call_response", id, response}(field name and required-ness verified against semconv v1.37.0; the role shaping is a documented deviation, see Decisions). Failed calls carry the wire'sis_erroras a sibling key; every cut part is markedfullsend.truncatedso fragments never read as whole results. Response bytes follow every existing invariant: redacted before any cut (assembly, eviction, pre-trim), exact dropped-byte accounting, tail-trim at the suffix boundary with theidsurviving.maxToolResultBytes = 8 KiBper-result bound, derived from measurement (below).internal/cli/tool_spans.go)execute_toolchild span per tool call under the iteration'sagentspan — started attool_usereceipt, ended attool_resultreceipt (runner clock at both ends); semconv v1.37.0 metadata only (gen_ai.operation.name,gen_ai.tool.name,gen_ai.tool.call.id,error.type=tool_erroronis_error). A call with no result by the end of the iteration is closed aserror.type=unansweredon every path that ends theagentspan, cancellation included; a result whose call was never reported is a near-zero-duration span markedfullsend.tool.unmatched; events without ids (pi, codex, and server-side tools whose result never arrives as atool_result) produce no span. Tool names pass through the same output sanitizer as span content (Unicode normalization, then secret redaction) and are bounded to 256 bytes for the attribute and 128 for the span name; at most 1,024 spans are recorded per iteration, with the overflow counted once per rejected call infullsend.tool_spans.droppedon theagentspan, so an agent-controlled burst cannot fill the OTLP batch queue and evict theagentspan. Emitted at every level — theOnEventtee is now always on, renderer first. Topology recorded in ADR 0108. Raised by rh-hemartin's review.No new configuration surface: still the one env var, zero knobs.
Measured basis for the 8 KiB cap
Three real review-agent runs from 2026-08-25 (main-thread transcripts):
Uncapped, two of three runs overflow the total budget and the suffix would evict whole older parts. The cap lowers eviction pressure; it does not prevent it — heavier iterations still overflow and evict oldest-first, marked exactly via
fullsend.content.truncated/dropped_bytes. These are main-thread transcript figures: the live stream also interleaves sub-agent results, so they are a lower bound on production volume (the gated review run below, which includes whatever sub-agent activity the stream carries, landed just under the band).Open to reviewer input: a capped response keeps its tail (the ordered-suffix policy extended per-result). No consumer requirement has confirmed tail vs. head for individual results — Bash output favors tails, file reads favor heads. Every cut part now carries
fullsend.truncated, which lowers the stakes: a scorer can see it holds a fragment. The direction stays a one-line change if scorer experience says otherwise.Decisions
{type, id, is_error: true, response: ""}(a custom marshaler guarantees the schema-requiredresponsekey on every response part). Bare credentials in results are covered: the redactor gainsya29.and bare-JWT patterns — the token classes WIF-provisioned runs actually handle. These patterns are repo-wide (the same redactor sanitizes forge comments and console output, not only span content — a stated decision, not a rider); measured over 2.1 MB of this repo's recent review and issue comment bodies, both patterns hit zero times. Theya29class excludes dots — with an optional one- or two-letter type segment covering the service-account (c.) and STS-minted (dr., per Google's workforce doc) shapes — so a mid-sentence token cannot swallow adjacent prose. The PostToolUse hook mirror of these shapes, with its checkout-scoped skip for the bare-JWT pattern, is split out to fix(security): mirror credential shapes into the PostToolUse hook, scoped to checkout content #7009 at review request (it changes the agent's live tool-call path, not the export path). Capped results are scanned exactly once (no double-counted findings), redaction shrink alone never marks a part truncated, and a result whose non-text blocks were skipped at flattening carriesfullsend.truncatedso the text fragment never reads as the whole result.role:"tool"ingen_ai.input.messages; part-type admission here is schema-valid) and cardinality (the registry note ties each output message to exactly one generation; this record packs the iteration's generations into one message). Rationale for both: stream order is preserved and the iteration has exactly one meaningfulfinish_reason(OutputMessagerequires one per message). Per-generation messages, if a consumer ever needs them, are a deliberate carrier change.execute_toolchild spans follow semconv v1.37.0, where the span is metadata-only; tool results (and, in PR C, arguments) stay ongen_ai.output.messages, the scorer contract, so nothing is duplicated across carriers. Timing is runner-side receipt at both ends: theagentspan's own clock, so a child never falls outside its parent through cross-host skew, and the one source every runtime provides (Claude Code'sassistantanduserstream lines carry a sandbox-clocktimestampthe parser does not decode; pi's tool-execution lines and codex's items carry none). The cost is bracketing that trails the sandbox by the pipe latency, and a start that is arguments-complete; decoding Claude Code's timestamps is recorded in ADR 0108 as open. Because these spans are Level 1, theOnEventtee is now always installed with the renderer first; the nil-handler invariant from feat(telemetry): implement ADR 0050 Level 3 content capture #6429 no longer holds and its test was replaced. ADR 0108 records the topology and the runtime-native OTel route it declines for now.Handleso they can neither accumulate nor serialize.tool_execution_end) but discards them on success by design; the claude/pi matrix indocs/runtimes.mdrecords the gap. Wiring pi is a natural follow-up.parent_tool_use_id(still dropped), and pi-runtime wiring — a two-change follow-up: passToolCallIDinto pi'sToolUseEventemission, then emitToolResultEventfromtool_execution_end(the payload is already decoded there).Evidence (corp MLflow exp 1)
minimal-exploreruntr-f6de9770a9ed5bb7af3b745576c70df1— 8 parts (2 reasoning / 2tool_call/ 2tool_call_response/ 2 text), ids correlated 2/2 (toolu_vrtx_…),responsekey on every response part, per-result cap fired live (dropped_bytes=16636,truncated=true), 3 redactions; content byte-verified inrun-telemetry.jsonland in the backend trace artifactreviewrun against this PR (tool-result-heavy)tr-9ad3bbacafc24d3adf88d3ea07cf5e49— 110 parts (14 reasoning / 47tool_call/ 47tool_call_response/ 2 text), ids correlated 47/47,responsekey on every part, attribute 125,076 bytes (just under the measured 127–255 KB band), cap live at scale (dropped_bytes=28497, largest kept responses exactly 8,192 bytes = the cap), 14 redactions,finish_reason=stop; validation passed against the agents-repo schema; content verified inrun-telemetry.jsonland on the backendfed318481333d228355e461b8f1a0ef2— spans emitted, zerogen_ai.output.messages, zerotool_call_response, zerofullsend.content.*markersminimal-explorerun with tool spanstr-b85f4a32161c0aa7663a1a48ae7f12af— 2execute_toolspans (Read,Write), both children of the iteration'sagentspan, kind Internal,gen_ai.operation.name/gen_ai.tool.name/gen_ai.tool.call.id(toolu_vrtx_…), status Ok, 64–410 ms;fullsend.tool_calls=2equals the two children; content (9,899 bytes) only on theagentspan, none on tool spans; parentage and status verified on the backend artifacte67a9098291728addf74f8ed41975093— 3execute_toolspans (Read×2,Write) still emitted as children of theagentspan, status Ok, 62–377 ms,fullsend.tool_calls=3equals the children; zerogen_ai.output.messagesand zerofullsend.content.*anywhere — the spans are Level 1, the content stays gatedreviewrun against this PR with tool spans (tool-heavy)tr-4182b037b48e8ea1d1236f3ecf9b98ec— 47execute_toolspans in one iteration (Bash×33,Read×13,Skill×1), every one a child of theagentspan, kind Internal;fullsend.tool_calls=47equals the 47 children; 46 status Ok and one real failure carryingerror.type=tool_errorwith status Error; durations 43 ms – 1.4 s; zero unanswered, zero unmatched; the iteration's 150,175-byte content record rides theagentspan only; validation passed; parentage and statuses verified on the backend artifactKnown limitations
userline whose tool_result carries e.g. a base64 image block exceeds the line buffer and its event is never emitted, sofullsend.content.truncated/dropped_bytescannot mark the loss and the correlated call stays unanswered (itsexecute_toolspan closes aserror.type=unansweredwith status Error, although the call itself may have succeeded). Noted in the parser; degraded extraction (id + leading text) is a candidate follow-up.</>/&) ride on top, so a budget-binding iteration serializes above 256 KiB — beyond the backend's 255 KB live-validated point (measured overhead on the evidence run: ~11%). Pre-existing semantics from feat(telemetry): implement ADR 0050 Level 3 content capture #6429 whose headroom tool results consume; serialized-size budgeting (or validating larger attributes) is a candidate follow-up.Next in this series (PR C, starts after this merges)
Input capture:
gen_ai.input.messageson retry-iteration agent spans, carrying the runner-composed validation feedback prompt (a real runner-side input since #6502; first iterations have none). Runner-side attachment only — no parser work — through the same gate and redaction pipeline, still no new surface. Also full toolargumentson the message record'stool_callparts (the schema's optional field; the parser already holds the input JSON) — redacted before any cut and bounded per part, sinceWriteinputs carry whole files. Serial like this PR: it starts once this merges, and it closes the series at three PRs.Tests
TDD throughout (every behavior red-first; mutation checks on the flat-shape fallback, tool_call id passing, the redact-before-cap ordering, and every tool-span policy branch — unanswered, orphan, malformed id, duplicate id, bounds, status, nil safety). Tool-span tests use the SDK's span recorder (parent, kind, attributes, status) plus an end-to-end file-sink check of
parentSpanId. Patch coverage on touched functions 88.7–100% (the tracker at 100%; the only uncovered patch lines are the wiring insiderunAgent, which has no runtime seam). Full-racesuite green.