fix(api): close conversation when dialogue stage reaches ended#126
Merged
Conversation
The bug was in the conversation completion path. Previously, the backend only finalized a conversation when llmResult.ended === true. In some cases, the dialogue state had already advanced to updatedState.stage === 'ended', but the ended flag was still false. When that happened, the system failed to submit the linked submission and failed to mark the conversation as closed. As a result, Firestore could remain in a half-finished state with the submission still marked in_progress, which prevented the frontend from showing the analysis / assessment screen because the UI gates that view on conversation.state === 'closed'. The fix was to unify the completion condition so the backend now closes the conversation whenever either signal says the dialogue is finished: llmResult.ended || llmResult.updatedState.stage === 'ended'. I also added regression tests to cover the broken case where the stage is ended but the boolean flag is not, and to confirm the existing normal completion path still works.
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a backend conversation-finalization bug where conversations could remain open (and submissions remain in_progress) when the dialogue state had reached stage === 'ended' but the ended boolean flag was still false. The change unifies completion detection and adds regression tests to prevent Firestore conversations/submissions from getting stuck in a half-finished state.
Changes:
- Normalize LLM completion detection to treat either
result.endedor an ended dialogue stage as completion. - Move conversation closing to
ConversationService(explicitupdateConversation({ state: 'closed' })) and remove theendedparameter fromappendTurns. - Add unit tests covering the “stage ended but ended flag false” completion path.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/mentora-api/tests/conversation-service-asr.unit.test.ts | Adds regression tests to assert submission + conversation closure when stage is ended even if ended flag is false. |
| packages/mentora-api/src/lib/server/repositories/ports/conversation-repository.ts | Removes ended from the appendTurns repository port. |
| packages/mentora-api/src/lib/server/repositories/firestore/conversation-repository.ts | Stops appendTurns from changing conversation state to closed. |
| packages/mentora-api/src/lib/server/llm/llm-service.ts | Computes a combined ended signal from handler output + orchestrator state. |
| packages/mentora-api/src/lib/server/application/conversation-service.ts | Closes the conversation when either completion signal indicates the dialogue is finished; adds additional logging. |
Comments suppressed due to low confidence (1)
packages/mentora-api/src/lib/server/repositories/firestore/conversation-repository.ts:173
appendTurnsno longer has a way to atomically transition the conversation toclosedwhile appending the final turns (it always writesstate: latestConversation.state). BecauseConversationService.addTurncloses the conversation later via a separateupdateConversationcall, there’s now a race window where anotheraddTurncan read an open conversation and append extra turns after the dialogue has effectively ended. Consider reintroducing ashouldCloseflag (or a dedicatedfinalizeTurn/appendTurnsAndClosemethod) so the final turn append + state transition happen in the same transaction.
transaction.update(conversationRef, {
turns: [...latestConversation.turns, ...params.turns],
state: latestConversation.state,
lastActionAt: params.finalNow,
updatedAt: params.finalNow,
tokenUsage: conversationTokenUsage
});
Comment on lines
595
to
+599
| await this.submitSubmission(assignment, user.uid, { | ||
| assessment: llmResult.assessment ?? undefined, | ||
| assessmentError: llmResult.assessmentError | ||
| }); | ||
| const closeNow = Date.now(); |
Comment on lines
+118
to
+121
| console.log('[ConversationService:submitSubmission]', { | ||
| assignmentId: assignment.id, | ||
| userId | ||
| }); |
Comment on lines
+541
to
+545
| dialogueStage: llmResult.updatedState.stage, | ||
| stageEndedButNotClosed: llmResult.updatedState.stage === 'ended' && !llmResult.ended | ||
| }); | ||
|
|
||
| const conversationShouldClose = llmResult.ended || llmResult.updatedState.stage === 'ended'; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug was in the conversation completion path. Previously, the backend only finalized a conversation when
llmResult.ended === true. In some cases, the dialogue state had already advanced to updatedState.stage ===
'ended', but the ended flag was still false. When that happened, the system failed to submit the linked
submission and failed to mark the conversation as closed. As a result, Firestore could remain in a half-finished
state with the submission still marked in_progress, which prevented the frontend from showing the analysis /
assessment screen because the UI gates that view on conversation.state === 'closed'.
The fix was to unify the completion condition so the backend now closes the conversation whenever either signal
says the dialogue is finished: llmResult.ended || llmResult.updatedState.stage === 'ended'. I also added
regression tests to cover the broken case where the stage is ended but the boolean flag is not, and to confirm
the existing normal completion path still works.