feat(events): carry tool outcome (duration/size/error) on ToolCompleted#18
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR extends ChangesToolCompleted Outcome Metadata
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Model
participant AgentLoop
participant Tool
participant EventsSink
participant LangfuseExporter
Model->>AgentLoop: request tool call
AgentLoop->>Tool: call()
Tool-->>AgentLoop: ToolResult(content, error)
AgentLoop->>AgentLoop: compute duration_ms, output_bytes, error
AgentLoop->>EventsSink: emit ToolCompleted{duration_ms, output_bytes, error}
EventsSink->>LangfuseExporter: forward event
LangfuseExporter->>LangfuseExporter: derive endTime, metadata, level/statusMessage
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c4a687f07
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // `error` mirrors `ToolResult::error` (`None` == success). | ||
| let duration_ms = crate::harness::ids::now_ms().saturating_sub(prepared.started_at_ms); | ||
| let output_bytes = result.content.len() as u64; | ||
| let error = result.error.clone(); |
There was a problem hiding this comment.
When RunPolicy::capture.tool_io is false (the default), the PayloadCapture contract says ToolCompleted stays payload-free, but this clones ToolResult::error into the event unconditionally. Since ToolResult::error() duplicates the model-facing tool result content into this field, any failed tool that includes secrets or PII in its error will now be journaled and exported (Langfuse maps it to statusMessage) even though tool input/output capture was disabled; gate the message behind the same capture policy or record only a non-sensitive failure flag.
Useful? React with 👍 / 👎.
AgentEvent::ToolCompleted gained started_at_ms but still lacked the outcome an exporter needs, forcing consumers to read a live side-channel: success/failure, duration, and result size. That makes journal-backed (offline) exporters impossible — a replayed journal can't see the run's in-memory outcome map. Add three additive, backward-compatible fields, populated in finish_tool_call from the ToolResult the loop already has: - duration_ms: wall-clock (completion − started_at_ms) - output_bytes: result content length - error: ToolResult::error verbatim (None == success) Old journals deserialize unchanged (all #[serde(default, skip_if none)]); success is derived from error.is_none() so no bool-default trap. Enriches the crate Langfuse tool span to use them: end time = start + duration (a real window, not the append timestamp), failed calls marked level=ERROR with the reason, and output_bytes in metadata. Tests: a failing tool's ToolCompleted carries error/duration/output_bytes; the Langfuse batch renders the duration-based end time and size. Unblocks C4: with the outcome on the journalled event, a journal-backed progress/trace projection can reach parity without the OpenHuman failure_map side-channel (see docs/tinyagents-full-migration-plan/C4-journal-progress-parity-plan.md §2a).
9c4a687 to
e32f225
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e32f2258b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // journal-backed exporters render duration/size/success without the | ||
| // live run's state. Duration is wall-clock (completion minus start); | ||
| // `error` mirrors `ToolResult::error` (`None` == success). | ||
| let duration_ms = crate::harness::ids::now_ms().saturating_sub(prepared.started_at_ms); |
There was a problem hiding this comment.
When a turn uses the concurrent tool path (multiple tool calls and no tool-wrap middleware), execute_tools_concurrently awaits every future with join_all before calling finish_tool_call in call order. Computing duration_ms here therefore measures until fold time, so a fast tool that finished early is reported as running until the slowest sibling completes (and later tools also include earlier after_tool hooks), which corrupts the journal/Langfuse execution window for parallel tools. Capture the completion timestamp/elapsed duration inside each tool future before join_all returns instead.
Useful? React with 👍 / 👎.
1 participant
Summary
AgentEvent::ToolCompletedgainedstarted_at_msbut still lacked the outcomean exporter needs — success/failure, duration, and result size — forcing
consumers to read a live, in-run side-channel. That makes journal-backed
(offline / replayed) exporters impossible: a journal replay can't see the run's
in-memory outcome map.
Add three additive, backward-compatible fields, populated in
finish_tool_callfrom theToolResultthe loop already has:duration_ms— wall-clock (completion − started_at_ms)output_bytes— result content lengtherror—ToolResult::errorverbatim (None== success)Old journals deserialize unchanged (every field is
#[serde(default, skip_serializing_if = "Option::is_none")]); success isderived from
error.is_none(), so there is nobool-default trap.Also enriches the crate Langfuse tool span to use them: end time =
start + duration(a real execution window rather than the journal-appendtimestamp), failed calls marked
level: "ERROR"with the reason, andoutput_bytesin metadata.Motivation
Unblocks a downstream consumer (OpenHuman) that wants to build progress/traces
from the durable event journal instead of a live per-run outcome map — the
event must be self-describing for that to hit parity.
Tests
ToolCompletedcarrieserror/duration_ms/output_bytes.Commands run
cargo fmt --checkcargo clippy --all-targets -- -D warningscargo test(933 lib + integration, green)Summary by CodeRabbit
New Features
Bug Fixes