fix(tui): publish synthetic reject event when permission/question ask is interrupted#29352
Open
sjawhar wants to merge 1 commit into
Open
fix(tui): publish synthetic reject event when permission/question ask is interrupted#29352sjawhar wants to merge 1 commit into
sjawhar wants to merge 1 commit into
Conversation
When the underlying "ask" effect for a Permission or Question prompt is
interrupted (tool cancelled, session ended, parent killed, scope torn down)
rather than completing via a normal reply/reject, the Effect.ensuring
finalizer deleted the pending entry but never published a corresponding
terminal bus event. The TUI tracks prompt state via these events, so the
orphaned request stayed in sync.data.permission / sync.data.question
forever. Pressing Allow/Reject/typing a custom answer / pressing Esc all
issued SDK calls against a requestID that the server no longer had in
pending — the handler logs "reply for unknown request" and returns 200
OK silently with no event, so the TUI never dismissed and the prompt
became completely unresponsive (every option re-rendered the same view,
even exit was unreachable while the prompt held the input).
Fix: replace the unconditional pending.delete in the finalizer with an
Effect.gen that checks pending.has(id). The reply and reject paths delete
the entry themselves before publishing, so this guard fires only on the
interrupt path. When it does, delete the entry and publish a synthetic
terminal event so the TUI dismisses the orphan:
- question/index.ts publishes Event.Rejected
- permission/index.ts publishes Event.Replied with reply: "reject"
(Permission has no dedicated cancellation event; "reject" is the
accepted Reply value that signals dismissal)
Adds regression tests in both packages that fork the ask effect, wait
for it to be pending, interrupt the fiber, and assert the bus emits the
expected terminal event and the pending list is empty.
Contributor
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
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
1 participant
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.
Issue for this PR
Closes #28312
Type of change
What does this PR do?
When a permission or question ask is interrupted (tool cancel, session end, fiber kill), the cleanup finalizer used to just remove the pending entry without publishing anything. The TUI subscribes to
Event.Replied/Event.Rejectedto dismiss prompts, so without that final event the prompt stayed visible forever andEnterdid nothing because the underlying entry was already gone.This adds a synthetic reject publish inside the finalizer, but only when the entry is still in
pending— the reply path deletes the entry before publishing its event, so this only fires on the orphan path and never races with a real reply.How did you verify your code works?
test/permission/next.test.tsthat forksask(), interrupts the fiber, and asserts arejectevent was published.test/question/question.test.ts.bun typecheck+ focused tests pass locally.Checklist