Repository navigation
fix(core): publish step.streamed at the first provider event (TTFB), not stream end - #57179
mradwankhalil wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
needs:compliance
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 72 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
This changes what time.streamed means, and the code that reads it relies on the current meaning, so as it stands it's a regression.
Your updated golden test shows session.step.streamed now firing before session.text.started, so time.streamed becomes roughly the time to first byte. Two places read it as the end of the stream:
- The TUI tokens-per-second figure (
turnTokensPerSecondinpackages/tui/src/routes/session/rows.ts) divides output tokens bystreamed - created. That span would shrink to about the time to first byte, so tokens per second would show far too high. Commit 2a132d6 deliberately set this window to include provider wait. - The debug metrics (
packages/gui-extensions/src/debug/metrics.ts) computee2efromstarted → streamedand tokens per second fromanswer/first → streamed. Withstreamedbefore the first text, the generation time clamps to 0, tokens per second disappears ande2ecollapses to about the time to first byte.
Time to first token is already measured there from the first text, reasoning or tool part's start time. If the "waiting for first reply" timer in #57178 needs a first-byte time, a separate signal (or the first content part's time.created) would avoid changing step.streamed. That would also mean changing the event schema and its consumers deliberately. The issue's claim that time.streamed is never persisted isn't addressed here either, since message-updater.ts is unchanged.
Checked by reading the code that uses the event and running the packages/core runner and projector tests (43 pass) and typecheck (clean). I didn't run OpenCode itself: the simulated model reports no token usage, so tokens per second can't be shown in the TUI.
Public API: the session.step.streamed event and the persisted time.streamed field keep their shape, but the timestamp now marks the first provider event instead of the end of the stream. Clients and plugins that treat it as the stream end, including the TUI and the debug metrics, would compute wrong durations.
| return | ||
| } | ||
| yield* publisher.publish(event) | ||
| if (!streamedPublished) { |
There was a problem hiding this comment.
This moves time.streamed from the end of the stream to the first provider event. The TUI's turnTokensPerSecond (packages/tui/src/routes/session/rows.ts) and attemptMetrics in packages/gui-extensions/src/debug/metrics.ts both use it as the stream end, to measure duration and tokens per second. With this change the TUI would show tokens per second far too high, and the debug metrics would drop tokens per second because streamed now comes before text.started. A first-byte time would be better as its own signal, for example the first content part's time.created, which the debug metrics already use for time to first token. It also now publishes streamed even when the step later ends in an overflow error, which the old !overflowFailure guard avoided.
|
This pull request has been automatically closed because it was not updated to meet our contributing guidelines within the 72-hour window. Feel free to open a new pull request that follows our guidelines. |
github-actions
Bot
removed
the
needs:compliance
|
Thanks for the auto-close notice. I have added the requested database evidence to #57178. Any future change will keep |
Fixes #57178
Publish step.streamed on the first published provider event instead of after the stream completes, exactly once; keep the end-of-stream call guarded. Updates the recorded-runner golden event order. 44 pass / 0 fail; tsgo clean.
Added by the opencode bot (AI-generated).
Related: #46883, #49381
Public API: changes when session.step.streamed fires / what time.streamed means (first provider event instead of stream end); same shape, but clients computing durations from it get different results