Skip to content

test: cover provider error session events#34

Merged
moose-lab merged 1 commit into
mainfrom
hotfix/provider-error-session-test
May 30, 2026
Merged

test: cover provider error session events#34
moose-lab merged 1 commit into
mainfrom
hotfix/provider-error-session-test

Conversation

@moose-lab

Copy link
Copy Markdown
Owner

Summary

  • add runProviderSessionTurn regression coverage for provider non-200 responses
  • verify stream event order stays ["status", "message", "error", "status"]
  • verify the failed user message remains in session_messages and the session becomes failed

Tests

  • git diff --check origin/main..HEAD
  • bun run test src/core/tests/api-session-runtime.test.ts (165/165 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 2m 2s —— View job


PR Review

  • Gather context
  • Read changed files
  • Review for correctness, type safety, and test quality
  • Post findings

Findings

  • [nit] The two "status" events are only verified by type, not value — the test doesn't assert the first is { status: "running" } and the second is { status: "failed" }. If the implementation swapped the emitted status values the event-order assertion would still pass. Low risk here but reduces diagnostic clarity (api-session-runtime.test.ts:174).

  • [nit] No assertion on the error message content (the "error" event's message field or result.error). If the error text from the provider changes shape the test won't catch it (api-session-runtime.test.ts:173).

  • [nit] The fetch-throws (network error) path is a distinct failure mode from non-200 and isn't covered by any test yet. Out of scope for this PR but worth a follow-up.

Verdict: APPROVE

The test is correct and fills a real gap. I traced the implementation path for a 500 response through api-session-runtime.ts:

  1. validateProviderRuntimeConfig passes (key present) → user message inserted → status(running) emitted.
  2. runProviderChatCompletion resolves to { ok: false } from the 500 response.
  3. postRequestStatus is still "running" → no early return / no message deletion.
  4. message(user) emitted, then session set to "failed", error emitted, status(failed) emitted.

That matches the asserted sequence ["status", "message", "error", "status"] and the DB state (user message retained, session failed). The test faithfully verifies all three invariants stated in the PR description.


@moose-lab
moose-lab merged commit a1933a9 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