fix(responses): record first-output timing for tool-only streams (carry #6259) - #6392
Conversation
Constraint: Keep the existing once-only callback and empty/control-event exclusions in native and adapted Responses streams. Rejected: Match every event ending in .delta | control and echo payloads must not start output timing. Confidence: high Scope-risk: narrow Directive: First-output timing is a proxy observation, not the start of hidden reasoning or exact model decoding. Tested: 183 focused Bun tests; TypeScript typecheck; privacy scan; structure checks; documentation build; pre-fix regressions reproduce missing timing. Not-tested: Full repository suite; upstream live inference on this dev checkout.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (2)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughFirst-output timing now starts on nonempty function-argument and custom-tool-input deltas. Empty deltas and tool-start scaffolding do not start the timer. Tests and documentation cover these event rules. ChangesFirst-output timing
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk is identified. The change adds first-output timing for nonempty tool-input deltas, with tests and documentation describing the behavior. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 4 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
|
Maintainer integration record (MAINTAINERS.md, dev-only) — DRAFT from lane L3
|
Summary
Carries #6259 by @xyjk0511 onto current
devso it can get full hosted CI. The single commit is replayed unchanged and keeps its author.A Responses turn that produces only a tool call never recorded a first-output time, so the request log showed output tokens with an empty TTFT. First-output detection only counted text and reasoning deltas. This PR also counts non-empty tool input:
firstOutputFromParsedinsrc/server/relay.ts) now counts non-emptyresponse.function_call_arguments.deltaandresponse.custom_tool_call_input.delta.src/bridge/sse.ts) counts atool_call_deltawith non-emptyarguments.Empty deltas, tool-start scaffolding, lifecycle events, and steer/inject control frames still do not start the timer, and the timestamp is still reported once. The dashboard guide and the transport docs now say the value is the first output the proxy saw, not the start of hidden reasoning or a decode-speed measurement. Whitespace-only tool input counts as output, the same
length > 0rule that text and reasoning already use.Carried from #6259. Original author: @xyjk0511.
Co-authored-by: xyjk buchanliang@gmail.com
Verification
git cherry-pick f2771cfe5c..6f44cd646eontodev7b2deb8059. It applied without conflicts: 7 files, +64/-7, identical to fix: record first-output timing for tool-only Responses streams #6259.git diff --check,bun run scripts/privacy-scan.ts, andbun run structure:checkpass. None of the touched files is intests/fixtures/file-size-baseline.json.tool_call_delta.argumentsis typedstringinsrc/types/request.ts, so the new branch reads a defined field.tests/adapters/bridge.test.tsandtests/server/response-log-inspection.test.ts, both existing files.Checklist
Summary by CodeRabbit
Improvements
Documentation