Skip to content

fix: restore incremental Responses reasoning - #138

Merged
pierreraby merged 2 commits into
patlux:mainfrom
sandexzx:fix/responses-reasoning-stream
Oct 6, 2026
Merged

pierreraby merged 2 commits into
patlux:mainfrom
sandexzx:fix/responses-reasoning-stream

Conversation

@sandexzx

@sandexzx sandexzx commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Restore incremental reasoning on Command Code's Provider API Responses wire, reproduced with deepseek/deepseek-v4.1-flash-fast.

After #133 selected Responses for models advertising /responses, Command Code's response.reasoning.delta events stopped reaching Pi as thinking deltas. pi-ai 1.0.3 understands response.reasoning_text.delta, but not this non-standard alias. The final response.output_item.done still supplies the complete reasoning text, which explains why thinking appeared all at once at the end.

Changes

  • Normalize only valid response.reasoning.delta SSE payloads to response.reasoning_text.delta, including the matching SSE event: name, before delegating to pi-ai.
  • Apply the wrapper only after resolving the model's Responses wire; leave Chat Completions, Anthropic Messages, generate fallback, and 403 upgrade_required detection unchanged.
  • Process complete SSE frames incrementally, preserving UTF-8 boundaries, a leading BOM, LF/CRLF/CR framing, multiline data, cancellation, and input stream errors.
  • Leave standard events, malformed payloads, completion events, and persisted reasoning signatures unchanged. No model allowlist, dependency changes, or pi-ai patch is required.
  • Add normalizer tests and a real-Pi regression whose mock server cannot finish the reasoning item until both thinking deltas have reached the client. Assert the final text is not duplicated.
  • Await RPC child-process closure before deleting test session directories, fixing the observed intermittent ENOTEMPTY cleanup failure. Report PASS only after cleanup succeeds.

Validation

  • npm test, with PI_LOCAL_REQUIRED=1 and OMP_COMPAT_REQUIRED=1 and a temporary OMP installation supplied through OMP_BIN: all suites passed, including both real hosts against mock APIs.
  • npm run format:check.
  • git diff --check.
  • Three additional consecutive npm run test:pi-local runs: all exited successfully after cleanup.
  • npm pack --ignore-scripts followed by npm run test:release-package -- <tarball>, with OMP_BIN: the installed tarball passed the Pi and OMP mock suites.
  • I verified the fix in Pi with a live Command Code request to DeepSeek V4.1 Flash Fast: reasoning now streams incrementally instead of appearing all at once at the end.

pierreraby
pierreraby previously approved these changes Oct 6, 2026

@pierreraby pierreraby left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verified: real bug, confirmed live — the gateway emits response.reasoning.delta (37x on deepseek-v4.1-flash-fast, 0x canonical alias) and pi-ai 1.0.3 drops them, so thinking lands all-at-once on every Responses-routed model. I captured raw frames: {type, sequence_number, item_id, output_index: 0, content_index, delta: string} — integer output_index + string delta, so the normalizer's schema assumption holds against the real gateway, not just the mock.

Local on 7e0e831: typecheck, test-transport 13/13, test-models 28/28, test-pricing 8/8, pi-local PASS, prettier clean. A fresh-context reviewer also checked the SSE parsing, wrapper scoping (Responses-only, 403 fallback preserved), and streaming properties — sound.

Two non-blocking P2s for follow-up: (1) finish() emits a trailing unterminated frame raw instead of normalizing it; (2) on schema mismatch the frame passes through silently, reverting to all-at-once with no signal. Also: this PR's wait-for-close cleanup and main's retry loop (0.7.5) fix the same ENOTEMPTY flake from opposite ends — suggest keeping wait-for-close as primary plus a longer retry backstop (and extending the waiter to runPi's timeout path) rather than picking one.

Leaving the merge to the maintainers.

@pierreraby pierreraby left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-verified on 8a615be (merge of main + P2 hardening): typecheck, test-transport 16/16, pi-local PASS, prettier clean. The finish() normalization and schema-mismatch warning address my earlier P2s; wait-for-close + retry loop are combined as suggested. Merging once CI is green.

@pierreraby
pierreraby merged commit 8ba07f5 into patlux:main Oct 6, 2026
15 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