Skip to content

feat(ruby_llm): correlate evaluation traces and control delivery - #5

Merged
TonsOfFun merged 6 commits into
mainfrom
codex/evaluation-trace-context
Sep 13, 2026
Merged

TonsOfFun merged 6 commits into
mainfrom
codex/evaluation-trace-context

Conversation

@TonsOfFun

@TonsOfFun TonsOfFun commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Changes

Evaluation results need the IDs of the actual response and judge traces. Extend RubyLLM with_agent with scoped attributes, a completed-trace callback, and an optional synchronous delivery attempt. Nested scopes restore context even on errors, and judge calls can have their own agent identity.

Callback errors do not interrupt tracing or expose private exception messages. Ordinary application tracing retains its existing asynchronous behavior. Synchronous delivery uses the existing reporter failure policy; it does not promise successful remote ingestion.

Validation

  • Core and RubyLLM adapter suites: 59 tests, 171 assertions pass.
  • Changed Ruby files lint clean under Omakase.
  • Tests cover trace identity, nested/error restoration, callback failures, and delivery ordering without model or collector network calls.
  • Review fixes: a turn captures the scope it started under on its first round, so a turn closed later by flush! keeps its agent, attributes and callback, and an unscoped turn never adopts a later scope (7610bda, 8e6585e); synchronous: true delivers through Reporter#report(sync: true), which keeps the sampling and configuration checks report_now skips (7610bda); on_trace fires only for a trace the reporter accepted for delivery, and acceptance is not ingestion (8e6585e); BatchingReporter#report(sync: true) delivers its own traces directly instead of racing the flusher, and answers false when the payload cannot be built (37404a2, 175a182). Tests cover each.

Review notes

Backward-compatible API. Bumps both gems to 0.3.0 (ebbfec5) so the extension ships as a release consumers can pin; the RubyLLM adapter now requires core ~> 0.3 for Reporter#report(sync:). Tag v0.3.0 after merge to publish.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Moderate issues remain around sampling behavior and preserving scoped metadata.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Extends the RubyLLM adapter with scoped evaluation context, trace correlation, callbacks, and optional synchronous delivery.

Changes:

  • Adds scoped attributes, agent identities, and nested context restoration.
  • Adds completed-trace callbacks and synchronous delivery controls.
  • Adds tests and documentation for evaluation trace correlation.
File summaries
File Summary
docs/pull-requests/evaluation-trace-context.md Records PR scope and validation.
docs/milestones/evaluation-trace-context.md Tracks milestone progress.
docs/issues/evaluation-trace-context.md Describes the evaluation-trace issue.
docs/branches/evaluation-trace-context.md Records branch metadata.
adapters/ruby_llm/test/test_evaluation_context.rb Tests correlation, restoration, delivery, and callbacks.
adapters/ruby_llm/README.md Documents the evaluation API.
adapters/ruby_llm/lib/activeagents/telemetry/ruby_llm.rb Implements scoped context and delivery controls.
Review details

Suppressed comments (1)

adapters/ruby_llm/lib/activeagents/telemetry/ruby_llm.rb:215

  • on_trace runs before reporter.report applies the reporter's sampling/configuration checks. With a sample_rate below 1, the callback can therefore persist an ID for a trace that is dropped and never emitted, which is not always the actual emitted trace ID described by this API. Make the callback conditional on the reporter accepting the trace (or expose a sampling-aware completion hook) while preserving the callback's exception isolation.
          notify_trace(agent[:on_trace], trace)
  • Files reviewed: 7/7 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread adapters/ruby_llm/lib/activeagents/telemetry/ruby_llm.rb
Comment thread adapters/ruby_llm/lib/activeagents/telemetry/ruby_llm.rb Outdated
TonsOfFun and others added 2 commits September 12, 2026 20:55
Bumps the core gem and the RubyLLM adapter to 0.3.0 and moves the
unreleased changelog entries under that version, so the scoped
`with_agent` extension ships as a release consumers can pin instead of
a git branch.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GnvoL2E5F411mxoxTgsn8q
… delivery

A turn now snapshots the `with_agent` scope it starts under, so a turn
left open by a pending tool call and closed later by `flush!` (or by the
next chat) reports with that scope's agent, attributes, callback and
delivery mode instead of whatever scope is active at report time.

`synchronous: true` no longer routes through `Reporter#report_now`, which
skips the sampling and configuration checks. `Reporter#report` takes
`sync: true` instead, delivering in the calling thread after the same
checks as ordinary delivery, and returns whether the traces were
accepted; `BatchingReporter#report` flushes in the calling thread when
asked the same. The adapter fires `on_trace` only for an accepted trace,
so a caller never keeps the ID of a trace `sample_rate` dropped.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GnvoL2E5F411mxoxTgsn8q

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Fix the RubyLLM scope-capture fallback and clarify that callbacks indicate delivery acceptance, not guaranteed collector ingestion.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

adapters/ruby_llm/README.md:114

  • The callback is invoked when Reporter#report accepts the trace, but delivery failures are swallowed by the reporter and still return acceptance, so no collector write is guaranteed. This wording overstates the contract by saying the ID was sent; describe it as accepted for delivery and note that remote ingestion may fail.
The callback receives each trace the reporter accepted, so an evaluation result
can retain the exact trace ID that was sent. A trace dropped by `sample_rate` or

adapters/ruby_llm/lib/activeagents/telemetry/ruby_llm.rb:177

  • turn.agent is nil both when no with_agent scope was active and when the turn has not captured one, so this fallback can claim an older unscoped turn with a later scope. For example, an ordinary tool-pending turn can be flushed by starting a new chat inside with_agent("Judge"); the old candidate trace will then use the judge's name, callback, and synchronous mode. Preserve an explicit “no captured scope” state (or resolve the initial context when the turn starts) before falling back to the current scope.
          agent = turn.agent || Thread.current[AGENT_KEY] || resolve_agent(payload) || DEFAULT_AGENT
  • Files reviewed: 15/15 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread CHANGELOG.md Outdated
…nestly

A turn now captures the `with_agent` scope on its first round whether or
not one is active, and report time no longer consults the current scope,
so a turn that started unscoped keeps its own identity even when the chat
that flushes it runs inside a judge scope.

The changelog and README no longer say the callback's trace ID is one the
collector will store: the reporter accepted it for delivery, a failed
delivery is logged rather than announced, and an asynchronous delivery can
outlive a process that exits at once.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GnvoL2E5F411mxoxTgsn8q

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Fix the synchronous batching race and reconcile the changelog release metadata.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

CHANGELOG.md:3

  • This changes the changelog from Unreleased to a dated 0.3.0 release, but the PR description says no release/deployment is included and docs/milestones/evaluation-trace-context.md:7 still marks merge and release as pending. Please keep the entry unreleased or coordinate the version/date with the actual release so repository metadata does not claim a release before it happens.
## [0.3.0] - 2026-09-12
  • Files reviewed: 15/15 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread lib/activeagents/telemetry/batching_reporter.rb Outdated
…g the flusher

`BatchingReporter#report(sync: true)` appended to the buffer and then
flushed after releasing the mutex, so the background flusher could claim
the buffer first and the call returned before its own traces went out.
A synchronous call now delivers its own traces directly, blocking, and
leaves the buffer on its schedule.

The branch docs now state the release plan the changelog already
records: tag `v0.3.0` after the merge.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GnvoL2E5F411mxoxTgsn8q

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Synchronous batching can report success when payload construction fails, and validation counts need reconciliation.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

docs/issues/evaluation-trace-context.md:14

  • This validation count also conflicts with the PR description's 51 tests and 139 assertions; the branch currently documents 58 and 164. Please align the issue note with the authoritative suite result and update the PR metadata if 58/164 is the correct count.
Validation: core 38 tests / 93 assertions and RubyLLM adapter 20 tests / 71
assertions pass on Ruby 4.0.2. New tests cover actual trace-ID correlation, judge

lib/activeagents/telemetry/batching_reporter.rb:41

  • This returns true even when deliver_batch cannot build the payload: that helper rescues payload-building errors and returns nil, but this unconditional return still makes RubyLLM invoke on_trace for a trace that was never handed to deliver. Propagate a separate payload-build success result here while continuing to treat post-build delivery failures as accepted/logged.
        if sync
          deliver_batch(accepted, blocking: true)
          return true
  • Files reviewed: 15/15 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread docs/pull-requests/evaluation-trace-context.md Outdated
… built as not accepted

`BatchingReporter#report(sync: true)` returned true even when building the
payload raised, so the RubyLLM adapter announced a trace that never reached
delivery. The synchronous path now builds the payload first and answers
false, logging the error, when that fails; a delivery failure after a built
payload is still logged and counts as accepted, as in `Reporter#report`.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01E4eTvjQ9tjpDJjHfJNYXXc
@TonsOfFun
TonsOfFun requested a lite review from Copilot September 13, 2026 17:29

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@TonsOfFun
TonsOfFun marked this pull request as ready for review September 13, 2026 17:35
@TonsOfFun
TonsOfFun merged commit f401a02 into main Sep 13, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants