feat(tui,app): surface & correlate background-job elicitations (#3584)#3624
Draft
aheritier wants to merge 4 commits into
Draft
feat(tui,app): surface & correlate background-job elicitations (#3584)#3624aheritier wants to merge 4 commits into
aheritier wants to merge 4 commits into
Conversation
…#3584) App.Start registers an OnElicitationRequest sink that LocalRuntime's elicitationHandler calls synchronously and exactly once for every elicitation request (#3584). For a foreground request, the elicitation bridge also best-effort-sends the same event on the very RunStream channel that Run/Retry/RunWithMessage read from directly, so that copy was forwarded into a.events a second time -- opening two dialogs for one request. Skip *runtime.ElicitationRequestEvent in those three RunStream forwarding loops, mirroring the exclusion agent_delegation.go's runCollecting already applies for background sub-sessions sharing the bridge slot. The sink stays the sole, exactly-once route into a.events for both foreground and background-job requests; remote/no-sink consumers (RemoteRuntime/SSE) are unaffected since they never go through pkg/app at all. Adds TestReview_ForegroundElicitationIsNotDeliveredTwice, the reviewer's composed-path regression: a sink registered AND an active RunStream for the same request must still produce exactly one delivery. Fails with 2 before this change, passes with 1 after.
…y delivered them (#3584) The previous fix (2ad095a) made App.Run/Retry/RunWithMessage skip every *runtime.ElicitationRequestEvent read off RunStream unconditionally, to stop a foreground LocalRuntime elicitation opening two dialogs (once via the OnElicitationRequest sink, once via the elicitation bridge's best-effort RunStream copy). That skip was over-broad: RemoteRuntime's OnElicitationRequest is a documented no-op, so its RunStream copy is the ONLY delivery for a remote-backed session — the unconditional skip silently dropped it, leaving zero dialogs instead of one. Add LocalRuntime.MirrorsElicitationOnRunStream, a marker method exposing the one real structural difference between the two runtimes: LocalRuntime's sink delivery and RunStream copy carry the same event, RemoteRuntime's sink never fires at all. App gates the skip on an optional-capability check (mustSkipMirroredElicitation) against that marker instead of skipping unconditionally, so: - LocalRuntime (foreground): sink delivers once, RunStream copy is recognised as a duplicate and skipped -> exactly one dialog. - LocalRuntime (background job): unaffected, still sink-only as before. - RemoteRuntime / any runtime without the marker: RunStream is the sole delivery path and passes through untouched -> exactly one dialog. No shared mutable state or dedupe map is introduced (avoiding the App-side ElicitationID dedupe hazard #3584 already flagged and removed once): the gate is a static, per-runtime capability check, computed once per RunStream loop, requiring no locks across sends and no lifecycle bookkeeping. Adds TestReview_RemoteRuntimeElicitationIsStillDelivered, a RemoteRuntime- shaped mock (no-op OnElicitationRequest, elicitation delivered only via RunStream) asserting exactly one dialog; this fails (0 delivered) against 2ad095a's unconditional skip and passes with this change. TestReview_ForegroundElicitationIsNotDeliveredTwice is retained and updated so its composed mock implements the new marker, keeping the original foreground double-delivery regression covered (still exactly one dialog).
…e-runtime classification (#3584) Review of bff8560 found the tests too weak to trust: the composed/ remote regression tests drove independent marker-shaped mocks instead of the concrete production runtimes, and only App.Run was exercised even though Retry and RunWithMessage duplicate the exact same mustSkipMirroredElicitation gating. Mutation proof this closes: - Removing the marker from *runtime.LocalRuntime, or adding it to *runtime.RemoteRuntime, previously left `go test ./pkg/app ./pkg/runtime` green. Add TestMustSkipMirroredElicitation_ConcreteRuntimeClassification, asserting mustSkipMirroredElicitation against the real *runtime.LocalRuntime and *runtime.RemoteRuntime pointer types (a nil pointer of each suffices: the helper only type-asserts, it never calls a method on rt). - Breaking remote delivery in Retry, or local dedupe in RunWithMessage, previously left the same command green because only Run was covered by the composed/remote mocks. Add TestElicitationDeliveryAcrossEntryPoints, a table-driven test that drives Run, Retry, AND RunWithMessage through both a mirrored/local- shaped and an unmirrored/remote-shaped mock runtime, asserting exactly one dialog in every combination. Also centralize the forwarding decision that Run/Retry/RunWithMessage each duplicated into a single App.forwardRunStreamEvents helper (app.go), used by all three loops, so there is only one gating implementation left to break instead of three independently-maintained copies. Retry's extra UserMessageEvent suppression becomes an optional filter callback; Run and RunWithMessage pass nil. Verified both mutation gaps are closed: reverting either runtime-marker mapping now fails the classification test, and breaking Retry's or RunWithMessage's delivery now fails the matching TestElicitationDeliveryAcrossEntryPoints subtest, while every other existing test stays green in isolation (proving the old suite's blindness before this change).
aheritier
added
area/runtime
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.
feat(tui,app): surface & correlate background-job elicitations (#3584)
Summary
Surfaces elicitation (user-input) requests from concurrent background jobs in the TUI, and ensures each request opens exactly one dialog regardless of which runtime delivers it. Part 3 of the #3584 stack.
Stacked on #3587 (runtime core) — review the diff against that branch.
What changed
App.Startregisters theOnElicitationRequestsink so background-job elicitations (no live foreground RunStream) reach the UI.retainDetachedElicitations: a foreground stream stop no longer discards a still-live detached background job's queued elicitation (filtersPendingEventsby session ID).LocalRuntimefeeds both the sink and its RunStream bridge with the same foreground elicitation. The App's RunStream forwarding now skips the mirroredElicitationRequestEventonly for runtimes advertisingMirrorsElicitationOnRunStream()(LocalRuntime).RemoteRuntimehas a no-op sink and delivers solely via RunStream, so its events pass through — exactly one dialog in every case.Run/Retry/RunWithMessageshare oneforwardRunStreamEventshelper so the gating can't drift.Testing
task dev+-race— green.