Skip to content

fix: defer provider user stream event#33

Merged
moose-lab merged 1 commit into
mainfrom
hotfix/provider-runtime-stream-event
May 30, 2026
Merged

fix: defer provider user stream event#33
moose-lab merged 1 commit into
mainfrom
hotfix/provider-runtime-stream-event

Conversation

@moose-lab

Copy link
Copy Markdown
Owner

Summary

  • defer the API-mode user message stream event until after the post-provider session status check
  • keep killed-session streams aligned with DB rollback behavior
  • update api-session-runtime regression expectations for success and killed paths

Tests

  • git diff --check origin/main..HEAD
  • bun run test src/core/tests/api-session-runtime.test.ts (164/164 passed because the test script runs the full suite)
  • bun run typecheck

@claude

claude Bot commented May 30, 2026

Copy link
Copy Markdown

Claude finished @moose-lab's task in 1m 10s —— View job


Review

  • Read changed files
  • Analyze logic and edge cases
  • Post review

Findings

The change moves emit(sessionId, { type: "message", role: "user", ... }) from before the status(running) emit + API call to after the postRequestStatus killed-session guard. This correctly aligns the stream with DB state.

Success path (old → new):
message(user), status(running), …status(running), message(user), …

Killed path (old → new):
message(user), status(running)status(running) ✓ (user row deleted from DB, no stream event emitted — now consistent)

  • [nit] api-session-runtime.ts:118 — the user message event is now emitted after a potentially long API round-trip. Clients that render the user's own message via the stream (rather than optimistically) will see a blank turn until the response arrives. Not a bug given the current architecture, but worth a comment if the intended streaming consumer ever changes.

  • [should-fix] No test covers the !result.ok (provider HTTP error) path. In that path the user message IS kept in the DB and IS now emitted at line 118 before error/status(failed) — which is correct — but there's no regression guard for it. Fix this →

No correctness bugs, type-safety issues, or security concerns found. The logic is tight and the existing test updates accurately capture both the success and killed-session orderings.

Verdict: APPROVE

@moose-lab
moose-lab merged commit 18c7e78 into main May 30, 2026
4 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.

1 participant