Skip to content

fix: three recording bugs — silent drop, replay double-count, missing span_id#42

Merged
risjai merged 3 commits into
masterfrom
fix/three-recording-bugs
Apr 12, 2026
Merged

fix: three recording bugs — silent drop, replay double-count, missing span_id#42
risjai merged 3 commits into
masterfrom
fix/three-recording-bugs

Conversation

@risjai

@risjai risjai commented Apr 12, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes 3 bugs discovered during E2E testing with real LLM calls (via OpenRouter):

Bug 1 (Critical): Direct mode silently drops all OpenAI recordings when openai-agents is installed

  • Repro: pip install rewind-agent[all] then rewind_agent.init() + raw openai.OpenAI() calls
  • Root cause: _try_register_openai_agents() unpatched the OpenAI SDK monkey-patches when the Agents SDK was detected, assuming the TracingProcessor would handle recording. But the TracingProcessor only fires via Runner.run() — raw SDK calls were silently dropped.
  • Result: 0 steps recorded, session showed "Steps: 0, Tokens: 0"
  • Fix: Keep monkey-patches active regardless of Agents SDK presence. TracingProcessor now only creates spans (skips _record_generation to avoid duplicates). Tool/handoff steps share the Recorder's step counter to avoid numbering conflicts.

Bug 2 (Medium): Fork-and-execute replay double-counts steps in diff

  • Repro: Record a 3-step session, replay from step 2
  • Root cause: _try_replay_cached() created new step records for cached replays in the forked timeline. But get_full_timeline_steps() already inherits parent steps where step_number <= fork_at_step, causing double-counting.
  • Result: Diff showed 5 steps (2 inherited + 2 cached duplicates + 1 live), with "diverge at step 2" even though cached steps should be identical.
  • Fix: _try_replay_cached() no longer creates step records — only advances the counter and returns the cached response. The fork inherits parent steps via the Rust replay engine as designed.

Before: ═ Step 1 identical ≠ Step 2 modified → Step 4 only in replayed → Step 5 only in replayed
After: ═ Step 1 identical ═ Step 2 identical ≠ Step 3 modified

Bug 3 (Medium): LLM calls within @span() blocks don't get span_id assigned

  • Repro: Use with rewind_agent.span("agent"): and make LLM calls inside it
  • Root cause: _SpanContext tracked _current_span_id via ContextVar but Recorder._record_call() never read it.
  • Result: All steps had span_id = NULL — steps couldn't be mapped to their enclosing spans
  • Fix: _record_call() now reads _current_span_id from hooks.py and passes it to create_step().

Before: Steps with spans: 0/3
After: Steps with spans: 3/3 — span tree shows LLM calls nested under their agent spans

Changes

File Change
python/rewind_agent/patch.py Remove OpenAI unpatch code from _try_register_openai_agents()
python/rewind_agent/openai_agents.py No-op _record_generation(), share Recorder's step counter in _write_step()
python/rewind_agent/recorder.py Remove step creation from _try_replay_cached(), add span_id lookup in _record_call()
python/tests/test_recorder.py 6 new unit tests covering all 3 fixes

Test plan

  • 106 Python unit tests pass (was 100 — 6 new tests added)
  • All Rust tests pass (cargo test)
  • E2E Phase 1: Direct recording works with openai-agents installed (Bug 1 verified)
  • E2E Phase 2: Fork → replay → diff shows correct step counts (Bug 2 verified)
  • E2E Phase 5: LLM calls inside @span() have span_id set (Bug 3 verified)
  • E2E Phases 3-4, 6-7: Assertions, evaluation, snapshots, web API, stress tests all pass
  • ruff check clean

Made with Cursor

risjai added 2 commits April 12, 2026 15:45
… span_id

Bug 1 (Critical): When openai-agents SDK is installed, init() unpatched
the OpenAI SDK monkey-patches in favor of the Agents SDK TracingProcessor.
But raw openai.OpenAI() calls (not via Runner.run()) were never captured
by the TracingProcessor, silently dropping all recordings (0 steps, 0 tokens).

Fix: Keep monkey-patches active regardless of Agents SDK presence. The
TracingProcessor now only creates spans (skips _record_generation to avoid
duplicate steps). Tool/handoff steps share the Recorder's step counter.

Bug 2 (Medium): Fork-and-execute replay created duplicate step records.
_try_replay_cached() wrote new steps to the fork timeline, but
get_full_timeline_steps() also inherited parent steps — producing phantom
extra steps in diff output and incorrect divergence points.

Fix: _try_replay_cached() no longer creates step records; it only advances
the step counter and returns cached responses. The fork timeline inherits
parent steps via the Rust replay engine as designed.

Bug 3 (Medium): LLM calls made within @span() blocks had span_id = NULL.
The _SpanContext tracked _current_span_id via ContextVar, but the Recorder's
_record_call() never read it.

Fix: _record_call() now reads _current_span_id from hooks.py and passes it
to create_step(), linking LLM call steps to their enclosing spans.

6 new unit tests cover all three fixes.

Made-with: Cursor
Patch release for three recording bug fixes (no Rust changes).

Made-with: Cursor

@risjai risjai left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Code Review

Overview

Fixes three real bugs discovered during E2E testing: (1) silent drop of OpenAI recordings when Agents SDK is co-installed, (2) double-counted steps in fork replays, (3) missing span_id on LLM calls inside @span() blocks. Excellent PR description with clear repro/root-cause/fix for each bug. +191 / -69, 6 new tests.

All three fixes are correct and well-tested. A few items worth addressing:

Issues

1. Reaching into Recorder internals (Medium)
openai_agents.py:245-252_write_step directly accesses self._recorder._lock and self._recorder._step_counter. This creates tight coupling between RewindTracingProcessor and Recorder internals.

Consider exposing a method on Recorder:

def next_step_number(self) -> int:
    """Atomically increment and return the next step number."""
    with self._lock:
        self._step_counter += 1
        return self._step_counter

2. Per-call import for span_id (Low)
recorder.py:593-596from .hooks import _current_span_id runs on every LLM call. Python caches module imports so overhead is minimal, but a module-level lazy import or caching the reference on first use would be cleaner.

3. Silent exception swallow on span_id lookup (Low)
recorder.py:596-597 — The bare except Exception: pass hides import failures or ContextVar issues. Suggest at minimum:

except Exception:
    logger.debug("Rewind: could not resolve span_id", exc_info=True)

4. Bug 1 test could be stronger (Low)
test_recorder.py:427-435test_openai_patches_preserved_with_agents_sdk only asserts keys exist in _originals. It doesn't verify the patches are still active on the class (i.e., that Completions.create is the patched version). A stronger assertion would check Completions.create != original.


Overall: solid fixes, clean diffs, good test coverage. None of the above are blockers — #1 is worth a follow-up to reduce coupling.

- Add Recorder.next_step_number() to avoid reaching into internals
- Cache _current_span_id import via module-level lazy helper
- Replace bare except with logger.debug for span_id lookup failures
- Strengthen Bug 1 test to verify patches are active on the class

Made-with: Cursor
@risjai
risjai merged commit 5b3e5ea into master Apr 12, 2026
4 checks passed
@risjai
risjai deleted the fix/three-recording-bugs branch April 12, 2026 10:24
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.

1 participant