Repository navigation
Conversation
Application-created fibers lose workflow and step attribution even when their calls belong to the enclosing workflow. Use inheritable fiber storage and clear workflow context in independent MCP listeners. Cover nested and concurrent work, usage attribution, and listener restart without changing the public workflow API. Fixes crmne#1323
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Fiber-local context and propagation lib/ruby_llm/support/instrumentation.rb, spec/ruby_llm/workflow_spec.rb, spec/ruby_llm/accounting/usage_workflow_spec.rb, docs/_advanced/instrumentation.md |
Workflow context now uses Fiber[] and restores the caller’s prior context. Specs cover child fibers, threads, concurrent steps, usage attribution, and disabled inheritance. Documentation describes child context and the limits of propagation. |
Listener context reset lib/ruby_llm/mcp/listener.rb, spec/ruby_llm/mcp/listener_spec.rb |
The listener thread runs with a nil workflow context. A spec checks that callbacks see no current workflow across two starts. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix · Severity of issue fixed: Medium
Possibly related PRs
- crmne/ruby_llm#1300: Usage entries persist workflow fields from the instrumentation context that this PR changes.
Suggested labels: bug, documentation
Suggested reviewers: crmne, kieranklaassen
Merge Risk: 
Minimal · up to 78335
-
No concrete merge-blocking issue is established by the supplied evidence. Merge after normal checks.
Pre-merge checks: 
6 · 
1
-
Failed checks: 1 warning Passed checks: 6 passed-
Full details: Linked Issues check
-
Explanation
Requirements from
#1323(direct link, open):- Child fibers, Async tasks, and threads keep the enclosing workflow and step. Met.
lib/ruby_llm/support/instrumentation.rbnow stores the context withFiber[WORKFLOW_CONTEXT_KEY]incurrent_workflowandwith_workflow(previouslyThread.current[...]). Fiber storage is copied into child fibers and threads at creation, so children inherit the active workflow and step.instrumentmerges that context into payloads, sochat.ruby_llmandusage.ruby_llmevents from children carryworkflow_idandworkflow_step_id. - Sibling steps stay isolated and the caller's context is restored. Met.
with_workflowsaves the previous value and restores it inensure. Each child that enters its own step replaces its own copy, and the context hashes are frozen, so parent and sibling values are not changed. The PR adds specs for overlapping Async steps and for parent-step restoration after errors (spec/ruby_llm/workflow_spec.rb, per the summary). I did not read these spec bodies. The result rests on the implementation and the PR report. - The MCP listener clears inherited context, as
#1186does for the usage owner. Met.lib/ruby_llm/mcp/listener.rbwraps the listener thread body inSupport::Instrumentation.with_workflow(nil). The listener spec (per the summary) checks that callbacks see no workflow on each start. - Background jobs re-enter with the persisted ID. This is out of automatic inheritance scope and needs no code change. The instrumentation guide was updated to say so (
docs/_advanced/instrumentation.md, per the summary).
No non-coding requirements apply to this issue. The PR reports a full test run with two failures in
spec/ruby_llm/sandbox/mxc_spec.rbthat the PR says also fail onmain. I did not run the suite. - Child fibers, Async tasks, and threads keep the enclosing workflow and step. Met.
Full details: Out of Scope Changes check
-
Explanation
Changes against the merge base
59df5a08, reviewed at head783357c4:lib/ruby_llm/support/instrumentation.rb: storage switched fromThread.currenttoFiber[]. This implements#1323directly.lib/ruby_llm/mcp/listener.rb: listener thread runs insidewith_workflow(nil), which implements the MCP-clearing requirement of#1323. ExistingProgressReporter.listenandAccounting::Usage.with_ownerwrappers are kept.docs/_advanced/instrumentation.md: documents child inheritance and the background-job re-entry rule. This supports the behavior change.- Specs in
spec/ruby_llm/workflow_spec.rb,spec/ruby_llm/accounting/usage_workflow_spec.rb, andspec/ruby_llm/mcp/listener_spec.rb: regression coverage for the changed behavior.
The PR states the concurrent tool executor keeps its explicit propagation unchanged. No diff hunk touches it, which matches that statement. No change is unrelated to
#1323. Public API signatures are unchanged.
Full details: Review Evidence Stays Out Of The Repository
-
Explanation
The pull request changes only one Markdown documentation file and five Ruby files. All six files are existing files; no screenshots, captures, recordings, comparison pages, media assets, or HTML evidence files are added. The documentation and specs contain ordinary workflow examples and tests, not reviewer evidence.
Full details: Dependencies From Sources The Maintainer Controls
-
Explanation
PASS. The PR changes only documentation, library code, and specs. It does not modify a manifest, lockfile, or package-manager configuration. The added
require 'async/queue'uses the existingasyncdevelopment dependency, which is unchanged in both base and head. No new git URL, fork, patch, override, replace entry, or alternate registry is introduced.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 5 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
What this does
Events from a child fiber,
Asynctask, or thread started insideRubyLLM.workflowlost their workflow and step, becauseSupport::Instrumentationkept the context in fiber-localThread.current[...]. Their usage attempts reached the ledger without the workflow they ran in.The workflow context now lives in fiber storage, the way
ProgressReporterkeeps the progress listener andAccounting::Usagekeeps the owner, so children inherit the workflow and step active when they are created. Workflow contexts are frozen hashes, so a child that enters its own step replaces its copy without touching its parent or siblings. The MCP listener thread clears the inherited workflow, as #1186 does for the usage owner, so notifications are not attributed to the workflow that happened to start the listener. The concurrent tool executor keeps its explicit propagation unchanged.The instrumentation guide now says that children inherit the context active when they are created, and that background jobs still re-enter
RubyLLM.workflowwith the persisted ID.The new examples cover:
Asyncembedding attempt is attributed to its workflow and step exactly once.Asyncsteps stay isolated from their siblings and the caller,Fiber.new(storage: nil)opts out, and the listener clears the workflow on every start.How to reproduce
Run from the repository root with
bundle exec ruby -Ilib repro.rb. Nothing goes over the network.Before:
After:
Type of change
Scope check
Related issue
Fixes #1323
Quality check
overcommit --installand all hooks passbundle exec rake vcr:record[provider_name]bundle exec rspec --tag '~live' --tag '~generator'(6030 examples; the only 2 failures, inspec/ruby_llm/sandbox/mxc_spec.rb, fail the same way onmainon macOS)docs/_advanced/instrumentation.mddescribes inheritance by child fibers and threads)models.json,aliases.json)Ownership
API changes
Summary by CodeRabbit