Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a fallback mechanism for identifying thread IDs in the web gateway channel by checking both the explicit thread_id field and the notify_thread_id key within the response metadata. The broadcast method was updated to utilize this new logic, and a test case was added to verify the metadata fallback. Feedback was provided to optimize the response_thread_id helper function for better consistency in handling empty strings and to reduce unnecessary cloning.
|
Can we get playwright test for this? |
standardtoaster
left a comment
There was a problem hiding this comment.
Heads up -- #2444 is also open against this same issue and takes a different approach: a DB fallback via get_or_create_assistant_conversation() when no thread_id is present, so broadcast always has somewhere to land. It's been approved by serrrfirat and is waiting on re-review after test additions.
Worth coordinating so we don't end up with conflicting fixes. Is there a scenario where notify_thread_id in metadata is needed beyond what the DB fallback would cover?
henrypark133
left a comment
There was a problem hiding this comment.
Review: this looks superseded by merged #2444
I checked the current PR against the surrounding review history. The original overlap concern from the thread is now concrete: #2444 has already merged, and it changed the same src/channels/web/mod.rs broadcast routing surface plus the same no_silent_drop tests with a different fix strategy (assistant-thread fallback for threadless broadcasts).
Because of that, I’m not comfortable approving this branch in its current form without first rebasing it onto current staging and re-evaluating what gap, if any, still remains after #2444.
Residual risk:
- As written, this PR is reviewing against an outdated contract for gateway routing, not just an isolated bugfix slice.
|
Thank you for addressing silently lost gateway responses caused by missing thread routing. We are closing this PR because Reborn now treats thread identity as a typed, persistent part of the execution and delivery scope. Reborn carries Your regression scenario is still valuable. We would be happy to have you contribute Reborn tests that prove a finalized reply remains bound to the correct thread across trigger execution, restart, or external delivery. Thank you for finding the silent-drop path. |
Resolves #2405 by allowing GatewayChannel::broadcast() to recover thread context from response.metadata.notify_thread_id before returning MissingRoutingTarget. This keeps explicit routing failures for truly unscoped messages while letting broadcasted responses that already carry routing metadata reach the correct thread.
Validation: