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 20 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 |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Reviewed via review-squad sweep + follow-up scrutiny of the OTel design (redaction-bypass finding already posted separately). Two more items below — neither blocks on its own, but the first changes what the agent's sandbox does on every tool call, which is out of scope for a telemetry PR.
| probe = parent | ||
|
|
||
|
|
||
| def content_skips(hook_input: dict) -> frozenset[str]: |
There was a problem hiding this comment.
This bare-JWT skip heuristic (cwd → nearest .git ancestor → normalize/resolve path → containment check) runs inside the agent's sandbox on every file-content tool call (Read/Edit/MultiEdit/Write/NotebookEdit/NotebookRead/Grep) — it changes what the model's PostToolUse hook does on the hot path, not just what gets emitted to telemetry.
Span/message-record content is already redacted runner-side (redactor.go) before it reaches a span or the message record, so this hook-side mirror isn't required for this PR's telemetry correctness — it's separate hardening (stated as "a stated decision, not a rider" in the PR body) riding along with the telemetry change. Given it touches the agent's live tool-call path rather than just the export path, could this be split into its own PR? That also makes it easier to review the parity argument (structural masking vs. bare-JWT skip) on its own merits.
Separately: internal/runtime/pi_extension/fullsend-hooks.js is touched in this PR even though pi tool-result emission is explicitly called out as out of scope ("Wiring pi is a natural follow-up") — worth clarifying why the pi hook needs a change here if pi isn't wired yet.
There was a problem hiding this comment.
Agreed — split into #7009: the two hook commits on their own branch; this PR keeps the Go redactor patterns, which its span content needs. The pi adapter change was a two-line header comment recording that the adapter sends no cwd, so the checkout skip is inert under pi; it goes with the hook PR.
| // spans when full, so an unbounded burst would evict the agent span — the | ||
| // one carrying the iteration's content. Half the queue leaves room for the | ||
| // rest of the trace; real review iterations run 117-255 calls. | ||
| const maxToolSpansPerIteration = 1024 |
There was a problem hiding this comment.
@rh-hemartin since you're the one who raised the span-vs-part question that led to execute_tool spans — flagging a low-priority follow-up for your read.
There's currently no way to disable or throttle execute_tool span emission short of OTEL_SDK_DISABLED=true (all telemetry off) — this cap is a pathological-case backstop, not a volume knob, and the sampler is hardcoded AlwaysSample(). Real review iterations run 117–255 spans per the PR's own numbers, versus ~1 agent span per iteration before this PR — that's a genuine increase in emission frequency this PR introduces (the file exporter's per-span synchronous write is pre-existing infra from #6429, not something to relitigate here, but this PR is what puts it on the per-tool-call path).
Worth either an explicit knob (env var / honoring OTEL_TRACES_SAMPLER on the OTLP path / configurable cap) or an ADR 0102 sentence explicitly accepting full per-call volume as the intended design — low severity either way, just want it to be a decision rather than a gap.
There was a problem hiding this comment.
Intentional, now stated in ADR 0102: full per-call volume is the design and the cap is a pathological-case backstop, not a knob. Sampling wouldn't do what a knob implies — the SDK samples per trace, head-based and parent-propagated, so honoring OTEL_TRACES_SAMPLER would drop whole runs from the OTLP path rather than thin these spans, and the file sink stays complete by design. A fullsend-specific switch would be a new configuration surface, which this series avoids; the levers are unsetting the endpoint (file only) and OTEL_SDK_DISABLED.
There was a problem hiding this comment.
We should accept the tool call volumes, and we are already doing batch exporter, so we should be good.
cc2846d to
88d4340
Compare
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>
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass focused on the new execute_tool span cap/drop accounting. One HIGH finding, inline below.
| if ok { | ||
| delete(t.open, id) | ||
| } else { | ||
| if !t.allow() { |
There was a problem hiding this comment.
HIGH — fullsend.tool_spans.dropped double-counts a single tool call past the span cap
allow() (lines 127-134) increments t.dropped unconditionally every time it's called while t.created >= maxToolSpansPerIteration, but it's invoked at two independent sites for what is logically one tool call:
- When that call's
ToolUseEventarrives past the cap (line 86) — the call returns without ever adding the id tot.open. - When that same call's
ToolResultEventlater arrives, finds no entry int.open(line 98), and falls into the unmatched branch that callsallow()a second time (line 102).
So for a call that is dropped for being over the cap, dropped is incremented twice — once on the use event, once on the result event — even though only one call was actually dropped. This contradicts the documented semantics in Finish()'s doc comment ("how many calls got no span") and in docs/guides/infrastructure/distributed-tracing.md.
TestToolSpanTracker_CapsSpansPerIteration (tool_spans_test.go:195-210) doesn't catch this: it sends maxToolSpansPerIteration+5 ToolUseEvents with no matching ToolResultEvent, plus one separate orphan-result id, so it never exercises the realistic case of maxToolSpansPerIteration+N complete use/result pairs. In that realistic case dropped reads 2N instead of N.
Suggestion: charge dropped once per logical call rather than once per event — e.g. record ids rejected on the ToolUseEvent path in a small rejected-set and skip the second allow() call on the ToolResultEvent unmatched branch when the id is already known-rejected, or otherwise restructure so a given id can only ever be counted once. Add a test that sends maxToolSpansPerIteration+N complete use/result pairs and asserts dropped == N.
There was a problem hiding this comment.
Fixed in 3348bd8. The overflow is charged once, at the rejected tool_use event; a result with no open span past the cap is not charged, 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 pairs past the cap and expects N.
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>
88d4340 to
3348bd8
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>
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=unanswered; 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 a literalc.alternative covering the service-account token shape — 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 —tool_uselines carry no timestamp andtool_resultlines carry a sandbox-clock one the parser ignores — one clock, approximate by design (the start is arguments-complete). 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.