Skip to content

feat(tui,app): surface & correlate background-job elicitations (#3584)#3624

Open
aheritier wants to merge 5 commits into
mainfrom
feat/tui-background-elicitations
Open

feat(tui,app): surface & correlate background-job elicitations (#3584)#3624
aheritier wants to merge 5 commits into
mainfrom
feat/tui-background-elicitations

Conversation

@aheritier

@aheritier aheritier commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

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 sink registration: App.Start registers the OnElicitationRequest sink so background-job elicitations (no live foreground RunStream) reach the UI.
  • Dialog/handler ID plumbing: the correlation ID is threaded through the dialog and resume handler so responses route back to the correct waiter.
  • retainDetachedElicitations: a foreground stream stop no longer discards a still-live detached background job's queued elicitation (filters PendingEvents by session ID).
  • No double delivery: LocalRuntime feeds both the sink and its RunStream bridge with the same foreground elicitation. The App's RunStream forwarding now skips the mirrored ElicitationRequestEvent only for runtimes advertising MirrorsElicitationOnRunStream() (LocalRuntime). RemoteRuntime has a no-op sink and delivers solely via RunStream, so its events pass through — exactly one dialog in every case. Run/Retry/RunWithMessage share one forwardRunStreamEvents helper so the gating can't drift.

Testing

  • Foreground local → exactly one dialog; remote/no-sink → still delivered via RunStream; concrete-type marker classification pinned; all three entry points covered.
  • A6 concurrent-background delivery/routing unaffected.
  • Full stack task dev + -race — green.

@aheritier aheritier mentioned this pull request Jul 13, 2026
Merged
@aheritier aheritier added area/runtime Runtime engine, agent loop execution, tool dispatch, loop detection area/tui For features/issues/fixes related to the TUI kind/feat PR adds a new feature (maps to feat:). Use on PRs only. labels Jul 13, 2026
Base automatically changed from fix/a6-elicitation-concurrency to main July 15, 2026 14:42
@aheritier aheritier force-pushed the feat/tui-background-elicitations branch from d2926c8 to 4b4ed27 Compare July 15, 2026 19:33
@aheritier aheritier marked this pull request as ready for review July 15, 2026 19:36
@aheritier aheritier requested a review from a team as a code owner July 15, 2026 19:36
@aheritier aheritier requested a review from docker-agent July 15, 2026 19:36
aheritier added 5 commits July 15, 2026 22:47
@aheritier
feat(tui,app): surface and correlate background-job elicitations in t…
2f45dbf 
…he TUI (#3584)
@aheritier
fix(app): stop delivering foreground elicitations twice into a.events (…
608cfe8 
…#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.
@aheritier
fix(app): only skip RunStream elicitation events when the sink alread…
a7f905a 
…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).
@aheritier
test(app): harden double-delivery regression coverage and pin concret…
44abc90 
…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
fix(tui,app): make elicitation-ID params variadic for backward compat…
f445d3e 
…ibility (#3584)

Change NewElicitationDialog, NewURLElicitationDialog,
NewOAuthAuthorizationDialog, and App.ResumeElicitation to take the new
correlation-ID argument as a trailing variadic elicitationID ...string
instead of a required positional string, following #3587's precedent for
Runtime.ResumeElicitation.

This restores source compatibility for external callers and fixes the
compile break against 3-arg NewElicitationDialog call sites added on main
(bebfc22). Existing call sites in this branch that pass a real
elicitation ID are unchanged (still pass it as the variadic arg), so
response correlation is preserved.

NewOAuthAuthorizationDialog's parameter order also changes: appInstance now
comes before the new variadic elicitationID, so the trailing ID stays last
across all four signatures.
@aheritier aheritier force-pushed the feat/tui-background-elicitations branch from af5ba07 to f445d3e Compare July 15, 2026 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@docker-agent docker-agent Awaiting requested review from docker-agent

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

area/runtime Runtime engine, agent loop execution, tool dispatch, loop detection area/tui For features/issues/fixes related to the TUI kind/feat PR adds a new feature (maps to feat:). Use on PRs only.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@aheritier