From 2f45dbff00d8cd0a7f4652d7af05cbd91df9b3cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arnaud=20He=CC=81ritier?= Date: Mon, 13 Jul 2026 14:09:12 +0200 Subject: [PATCH 1/5] feat(tui,app): surface and correlate background-job elicitations in the TUI (#3584) --- pkg/app/app.go | 20 +++- pkg/app/app_test.go | 107 ++++++++++++++++++ pkg/tui/dialog/base.go | 4 +- pkg/tui/dialog/elicitation.go | 20 ++-- pkg/tui/dialog/elicitation_test.go | 18 +-- pkg/tui/dialog/manager_test.go | 2 +- pkg/tui/dialog/oauth_authorization.go | 22 ++-- pkg/tui/dialog/url_elicitation.go | 26 +++-- pkg/tui/dialog/url_elicitation_test.go | 6 +- pkg/tui/handlers.go | 4 +- pkg/tui/messages/mcp.go | 4 + pkg/tui/page/chat/runtime_events.go | 6 +- pkg/tui/service/supervisor/supervisor.go | 34 +++++- pkg/tui/service/supervisor/supervisor_test.go | 83 ++++++++++++++ pkg/tui/tui.go | 8 +- 15 files changed, 303 insertions(+), 61 deletions(-) diff --git a/pkg/app/app.go b/pkg/app/app.go index b90d158861..fd1860eb65 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -194,6 +194,18 @@ func (a *App) Start(ctx context.Context) { case <-ctx.Done(): } }) + + // Forward elicitation requests raised anywhere in the runtime — + // including background-job (run_background_agent) sub-sessions whose + // RunStream has no live UI reading its own events channel — so they + // always reach the TUI as a dialog. This is the runtime's single, + // exactly-once delivery point for elicitation requests (#3584); the + // swap-based bridge is a separate best-effort path for remote/SSE + // consumers only and never also reaches this sink, so no App-side + // dedupe is needed here. + a.runtime.OnElicitationRequest(func(event runtime.Event) { + a.sendEvent(ctx, event) + }) }) } @@ -974,9 +986,11 @@ func (a *App) TogglePause() (paused, supported bool) { return p, true } -// ResumeElicitation resumes an elicitation request with the given action and content -func (a *App) ResumeElicitation(ctx context.Context, action tools.ElicitationAction, content map[string]any) error { - return a.runtime.ResumeElicitation(ctx, action, content) +// ResumeElicitation resumes an elicitation request with the given action and +// content. elicitationID is additive: pass "" to fall back to resolving the +// sole pending request (see runtime.Runtime.ResumeElicitation). +func (a *App) ResumeElicitation(ctx context.Context, action tools.ElicitationAction, content map[string]any, elicitationID string) error { + return a.runtime.ResumeElicitation(ctx, action, content, elicitationID) } func (a *App) NewSession() { diff --git a/pkg/app/app_test.go b/pkg/app/app_test.go index a2053d11a0..1ac9329c74 100644 --- a/pkg/app/app_test.go +++ b/pkg/app/app_test.go @@ -293,6 +293,113 @@ func TestApp_Start_ForwardsBackgroundEvents(t *testing.T) { } } +// elicitationRequestMockRuntime captures the handler App.Start registers via +// OnElicitationRequest and records every ResumeElicitation call, so tests can +// drive the sink and assert on what App forwards back to the runtime. +type elicitationRequestMockRuntime struct { + mockRuntime + + handler func(runtime.Event) + + mu sync.Mutex + resumedIDs []string + resumedActions []tools.ElicitationAction +} + +// firstOrEmpty returns the first element of ids, or "" when empty. Mirrors +// runtime.firstElicitationID for tests that record a variadic call's ID. +func firstOrEmpty(ids []string) string { + if len(ids) == 0 { + return "" + } + return ids[0] +} + +func (m *elicitationRequestMockRuntime) OnElicitationRequest(handler func(runtime.Event)) { + m.handler = handler +} + +func (m *elicitationRequestMockRuntime) ResumeElicitation(_ context.Context, action tools.ElicitationAction, _ map[string]any, elicitationID ...string) error { + m.mu.Lock() + defer m.mu.Unlock() + m.resumedIDs = append(m.resumedIDs, firstOrEmpty(elicitationID)) + m.resumedActions = append(m.resumedActions, action) + return nil +} + +// TestApp_Start_ForwardsElicitationRequests verifies Start wires the +// runtime's OnElicitationRequest sink into the app's event stream, so +// background-job elicitations (which have no live channel of their own +// reaching the TUI) are surfaced (#3584). +func TestApp_Start_ForwardsElicitationRequests(t *testing.T) { + t.Parallel() + + rt := &elicitationRequestMockRuntime{} + events := make(chan tea.Msg, 16) + app := &App{ + runtime: rt, + session: session.New(), + events: events, + } + + app.Start(t.Context()) + require.NotNil(t, rt.handler, "Start must register the OnElicitationRequest handler") + + ev := runtime.ElicitationRequest("need input", "form", nil, "", "eid-1", "", "sess-1", nil, "worker") + rt.handler(ev) + + select { + case msg := <-events: + assert.Equal(t, ev, msg, "the elicitation request must reach the app's event stream unchanged") + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for the forwarded elicitation request") + } +} + +// TestApp_SendEvent_DeliversElicitationWithoutDedupe pins the #3584 fix that +// removed the App-side ElicitationID dedupe: the runtime's +// OnElicitationRequest sink is now the single, exactly-once delivery point +// (elicitationHandler calls it directly, synchronously, and unconditionally, +// and runCollecting no longer re-forwards a bridge-observed copy), so +// sendEvent must forward every event unconditionally — including two +// distinct events that happen to share an ElicitationID, which a stateful +// dedupe would have incorrectly collapsed. +func TestApp_SendEvent_DeliversElicitationWithoutDedupe(t *testing.T) { + t.Parallel() + + events := make(chan tea.Msg, 16) + app := &App{events: events} + ctx := t.Context() + + ev := runtime.ElicitationRequest("need input", "form", nil, "", "eid-dup", "", "sess-1", nil, "worker") + app.sendEvent(ctx, ev) + require.Len(t, events, 1, "the sink's single delivery must reach the app's event stream") + <-events + + // A second event that happens to carry the same ElicitationID (e.g. a + // canceled request's ID reused later) must still go through: nothing in + // the App layer keys off ElicitationID any more. + app.sendEvent(ctx, ev) + require.Len(t, events, 1, "sendEvent must not drop a delivery based on ElicitationID") +} + +// TestApp_ResumeElicitation_ForwardsID verifies ResumeElicitation passes the +// elicitation ID through to the runtime unchanged. There is no dedupe state +// to clear any more (#3584): the runtime's per-request waiter registry is +// the sole source of truth for whether an ID is still answerable. +func TestApp_ResumeElicitation_ForwardsID(t *testing.T) { + t.Parallel() + + rt := &elicitationRequestMockRuntime{} + app := &App{runtime: rt, events: make(chan tea.Msg, 16)} + + require.NoError(t, app.ResumeElicitation(t.Context(), tools.ElicitationActionAccept, nil, "eid-clear")) + + require.Len(t, rt.resumedIDs, 1) + assert.Equal(t, "eid-clear", rt.resumedIDs[0]) + assert.Equal(t, tools.ElicitationActionAccept, rt.resumedActions[0]) +} + // stubSnapshotController is a tiny SnapshotController used by the app // tests to drive /undo without spinning up a real shadow-git // repository. enabled gates SnapshotsEnabled(), and the (files, ok, diff --git a/pkg/tui/dialog/base.go b/pkg/tui/dialog/base.go index 40065d74a9..6665b1bc43 100644 --- a/pkg/tui/dialog/base.go +++ b/pkg/tui/dialog/base.go @@ -101,10 +101,10 @@ func ContentEndRow(dialogRow, dialogHeight int) int { } // CloseWithElicitationResponse returns a command that closes the dialog and sends an elicitation response. -func CloseWithElicitationResponse(action tools.ElicitationAction, content map[string]any) tea.Cmd { +func CloseWithElicitationResponse(action tools.ElicitationAction, content map[string]any, elicitationID string) tea.Cmd { return tea.Sequence( core.CmdHandler(CloseDialogMsg{}), - core.CmdHandler(messages.ElicitationResponseMsg{Action: action, Content: content}), + core.CmdHandler(messages.ElicitationResponseMsg{Action: action, Content: content, ElicitationID: elicitationID}), ) } diff --git a/pkg/tui/dialog/elicitation.go b/pkg/tui/dialog/elicitation.go index 55bfe167b5..70d9c9f41a 100644 --- a/pkg/tui/dialog/elicitation.go +++ b/pkg/tui/dialog/elicitation.go @@ -59,6 +59,7 @@ type ElicitationDialog struct { title string message string + elicitationID string fields []ElicitationField inputs []textinput.Model boolValues map[int]bool @@ -119,7 +120,7 @@ func (d *ElicitationDialog) hasFreeFormInput() bool { } // NewElicitationDialog creates a new elicitation dialog. -func NewElicitationDialog(message string, schema any, meta map[string]any) Dialog { +func NewElicitationDialog(message string, schema any, meta map[string]any, elicitationID string) Dialog { fields := parseElicitationSchema(schema) // Determine dialog title from meta, defaulting to "Question" @@ -131,13 +132,14 @@ func NewElicitationDialog(message string, schema any, meta map[string]any) Dialo } d := &ElicitationDialog{ - title: title, - message: message, - fields: fields, - inputs: make([]textinput.Model, len(fields)), - boolValues: make(map[int]bool), - enumIndexes: make(map[int]int), - fieldErrors: make(map[int]string), + title: title, + message: message, + elicitationID: elicitationID, + fields: fields, + inputs: make([]textinput.Model, len(fields)), + boolValues: make(map[int]bool), + enumIndexes: make(map[int]int), + fieldErrors: make(map[int]string), keyMap: elicitationKeyMap{ Up: key.NewBinding(key.WithKeys("up")), Down: key.NewBinding(key.WithKeys("down")), @@ -384,7 +386,7 @@ func (d *ElicitationDialog) isTextInputField() bool { } func (d *ElicitationDialog) close(action tools.ElicitationAction, content map[string]any) tea.Cmd { - return CloseWithElicitationResponse(action, content) + return CloseWithElicitationResponse(action, content, d.elicitationID) } // collectAndValidate validates all fields and returns the collected values. diff --git a/pkg/tui/dialog/elicitation_test.go b/pkg/tui/dialog/elicitation_test.go index 9b86b73fe7..8555c11d8d 100644 --- a/pkg/tui/dialog/elicitation_test.go +++ b/pkg/tui/dialog/elicitation_test.go @@ -340,7 +340,7 @@ func TestNewElicitationDialog(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - dialog := NewElicitationDialog(tt.message, tt.schema, tt.meta) + dialog := NewElicitationDialog(tt.message, tt.schema, tt.meta, "") require.NotNil(t, dialog) ed, ok := dialog.(*ElicitationDialog) @@ -560,7 +560,7 @@ func TestElicitationDialog_collectAndValidate(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - dialog := NewElicitationDialog("test", tt.schema, nil).(*ElicitationDialog) + dialog := NewElicitationDialog("test", tt.schema, nil, "").(*ElicitationDialog) if tt.setupInputs != nil { tt.setupInputs(dialog) } @@ -588,7 +588,7 @@ func TestElicitationDialog_PasswordFieldMaskedAndUntrimmed(t *testing.T) { }, "required": []any{"password"}, } - dialog := NewElicitationDialog("sudo", schema, nil).(*ElicitationDialog) + dialog := NewElicitationDialog("sudo", schema, nil, "").(*ElicitationDialog) // The password input is masked, not echoed in clear text. assert.Equal(t, textinput.EchoPassword, dialog.inputs[0].EchoMode) @@ -616,7 +616,7 @@ func TestElicitationDialog_LongEnumScrolls(t *testing.T) { "enum": enumValues, } - dialog := NewElicitationDialog("Choose an option:", schema, nil).(*ElicitationDialog) + dialog := NewElicitationDialog("Choose an option:", schema, nil, "").(*ElicitationDialog) require.Len(t, dialog.fields, 1) require.Len(t, dialog.fields[0].EnumValues, 30) @@ -649,7 +649,7 @@ func TestElicitationDialog_FieldsBelowFold_AreReachable(t *testing.T) { } schema := map[string]any{"type": "object", "properties": props, "required": required} - dialog := NewElicitationDialog("Fill in the form", schema, nil).(*ElicitationDialog) + dialog := NewElicitationDialog("Fill in the form", schema, nil, "").(*ElicitationDialog) _, _ = dialog.Update(tea.WindowSizeMsg{Width: 100, Height: 18}) _ = dialog.View() @@ -681,7 +681,7 @@ func TestElicitationDialog_SmallContent_NoScrollbar(t *testing.T) { "required": []any{"name"}, } - dialog := NewElicitationDialog("Enter your name", schema, nil).(*ElicitationDialog) + dialog := NewElicitationDialog("Enter your name", schema, nil, "").(*ElicitationDialog) _, _ = dialog.Update(tea.WindowSizeMsg{Width: 100, Height: 30}) _ = dialog.View() @@ -708,7 +708,7 @@ func TestElicitationDialog_TypingRevealsBelowFoldField(t *testing.T) { "required": []any{"name"}, } - dialog := NewElicitationDialog(longMessage, schema, nil).(*ElicitationDialog) + dialog := NewElicitationDialog(longMessage, schema, nil, "").(*ElicitationDialog) _, _ = dialog.Update(tea.WindowSizeMsg{Width: 80, Height: 16}) _ = dialog.View() @@ -748,7 +748,7 @@ func TestElicitationDialog_OpensScrolledToTop(t *testing.T) { "enum": enumValues, } - dialog := NewElicitationDialog(longMessage, schema, nil).(*ElicitationDialog) + dialog := NewElicitationDialog(longMessage, schema, nil, "").(*ElicitationDialog) _, _ = dialog.Update(tea.WindowSizeMsg{Width: 80, Height: 18}) _ = dialog.View() @@ -770,7 +770,7 @@ func TestElicitationDialog_UserScrollUp_NotSnappedBack(t *testing.T) { } schema := map[string]any{"type": "string", "title": "Pick one", "enum": enumValues} - dialog := NewElicitationDialog(longMessage, schema, nil).(*ElicitationDialog) + dialog := NewElicitationDialog(longMessage, schema, nil, "").(*ElicitationDialog) _, _ = dialog.Update(tea.WindowSizeMsg{Width: 80, Height: 16}) _ = dialog.View() diff --git a/pkg/tui/dialog/manager_test.go b/pkg/tui/dialog/manager_test.go index c15fb5f92f..68e89fe474 100644 --- a/pkg/tui/dialog/manager_test.go +++ b/pkg/tui/dialog/manager_test.go @@ -32,7 +32,7 @@ func TestManagerBackgroundDialog(t *testing.T) { // Stack a background dialog (i.e. one carrying an originating event) on top. type fakeEvent struct{ id int } event := &fakeEvent{id: 42} - bg := NewElicitationDialog("Pick a value", nil, nil) + bg := NewElicitationDialog("Pick a value", nil, nil, "") mgr.handleOpen(OpenDialogMsg{ Model: bg, OriginatingEvent: event, diff --git a/pkg/tui/dialog/oauth_authorization.go b/pkg/tui/dialog/oauth_authorization.go index df0b1b3ecf..f3707d2847 100644 --- a/pkg/tui/dialog/oauth_authorization.go +++ b/pkg/tui/dialog/oauth_authorization.go @@ -18,18 +18,20 @@ type oauthAuthorizationDialog struct { ctx func() context.Context - serverURL string - app *app.App - keyMap ConfirmKeyMap + serverURL string + elicitationID string + app *app.App + keyMap ConfirmKeyMap } // NewOAuthAuthorizationDialog creates a new OAuth authorization confirmation dialog -func NewOAuthAuthorizationDialog(ctx context.Context, serverURL string, appInstance *app.App) Dialog { +func NewOAuthAuthorizationDialog(ctx context.Context, serverURL, elicitationID string, appInstance *app.App) Dialog { return &oauthAuthorizationDialog{ - ctx: func() context.Context { return context.WithoutCancel(ctx) }, - serverURL: serverURL, - app: appInstance, - keyMap: DefaultConfirmKeyMap(), + ctx: func() context.Context { return context.WithoutCancel(ctx) }, + serverURL: serverURL, + elicitationID: elicitationID, + app: appInstance, + keyMap: DefaultConfirmKeyMap(), } } @@ -52,11 +54,11 @@ func (d *oauthAuthorizationDialog) Update(msg tea.Msg) (layout.Model, tea.Cmd) { model, cmd, handled := HandleConfirmKeys(msg, d.keyMap, func() (layout.Model, tea.Cmd) { - _ = d.app.ResumeElicitation(d.ctx(), tools.ElicitationActionAccept, nil) + _ = d.app.ResumeElicitation(d.ctx(), tools.ElicitationActionAccept, nil, d.elicitationID) return d, core.CmdHandler(CloseDialogMsg{}) }, func() (layout.Model, tea.Cmd) { - _ = d.app.ResumeElicitation(d.ctx(), tools.ElicitationActionDecline, nil) + _ = d.app.ResumeElicitation(d.ctx(), tools.ElicitationActionDecline, nil, d.elicitationID) return d, core.CmdHandler(CloseDialogMsg{}) }, ) diff --git a/pkg/tui/dialog/url_elicitation.go b/pkg/tui/dialog/url_elicitation.go index 0c00c56396..dec734bb0a 100644 --- a/pkg/tui/dialog/url_elicitation.go +++ b/pkg/tui/dialog/url_elicitation.go @@ -20,21 +20,23 @@ type URLElicitationDialog struct { ctx func() context.Context - message string - url string - keyMap ConfirmKeyMap - escape key.Binding - openBrowser key.Binding + message string + url string + elicitationID string + keyMap ConfirmKeyMap + escape key.Binding + openBrowser key.Binding } // NewURLElicitationDialog creates a new URL elicitation dialog. -func NewURLElicitationDialog(ctx context.Context, message, url string) Dialog { +func NewURLElicitationDialog(ctx context.Context, message, url, elicitationID string) Dialog { return &URLElicitationDialog{ - ctx: func() context.Context { return context.WithoutCancel(ctx) }, - message: message, - url: url, - keyMap: DefaultConfirmKeyMap(), - escape: key.NewBinding(key.WithKeys("esc")), + ctx: func() context.Context { return context.WithoutCancel(ctx) }, + message: message, + url: url, + elicitationID: elicitationID, + keyMap: DefaultConfirmKeyMap(), + escape: key.NewBinding(key.WithKeys("esc")), openBrowser: key.NewBinding( key.WithKeys("o"), key.WithHelp("o", "open"), @@ -79,7 +81,7 @@ func (d *URLElicitationDialog) Update(msg tea.Msg) (layout.Model, tea.Cmd) { } func (d *URLElicitationDialog) respond(action tools.ElicitationAction) tea.Cmd { - return CloseWithElicitationResponse(action, nil) + return CloseWithElicitationResponse(action, nil, d.elicitationID) } func (d *URLElicitationDialog) openURLInBrowser() tea.Cmd { diff --git a/pkg/tui/dialog/url_elicitation_test.go b/pkg/tui/dialog/url_elicitation_test.go index 3991cc1012..4be4d33af1 100644 --- a/pkg/tui/dialog/url_elicitation_test.go +++ b/pkg/tui/dialog/url_elicitation_test.go @@ -30,7 +30,7 @@ func TestNewURLElicitationDialog(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - dialog := NewURLElicitationDialog(t.Context(), tt.message, tt.url) + dialog := NewURLElicitationDialog(t.Context(), tt.message, tt.url, "") require.NotNil(t, dialog) ud, ok := dialog.(*URLElicitationDialog) @@ -44,7 +44,7 @@ func TestNewURLElicitationDialog(t *testing.T) { func TestURLElicitationDialog_View(t *testing.T) { t.Parallel() - dialog := NewURLElicitationDialog(t.Context(), "Please visit the URL", "https://example.com/callback").(*URLElicitationDialog) + dialog := NewURLElicitationDialog(t.Context(), "Please visit the URL", "https://example.com/callback", "").(*URLElicitationDialog) dialog.SetSize(100, 50) view := dialog.View() @@ -61,7 +61,7 @@ func TestURLElicitationDialog_View(t *testing.T) { func TestURLElicitationDialog_HasOpenKeyBinding(t *testing.T) { t.Parallel() - dialog := NewURLElicitationDialog(t.Context(), "Test", "https://example.com").(*URLElicitationDialog) + dialog := NewURLElicitationDialog(t.Context(), "Test", "https://example.com", "").(*URLElicitationDialog) // Verify the openBrowser key binding exists and is configured correctly require.NotNil(t, dialog.openBrowser) diff --git a/pkg/tui/handlers.go b/pkg/tui/handlers.go index aed763575b..1ba506a9b7 100644 --- a/pkg/tui/handlers.go +++ b/pkg/tui/handlers.go @@ -1214,8 +1214,8 @@ func (m *appModel) closeTranscriptCh() { } } -func (m *appModel) handleElicitationResponse(action tools.ElicitationAction, content map[string]any) (tea.Model, tea.Cmd) { - if err := m.application.ResumeElicitation(m.ctx(), action, content); err != nil { +func (m *appModel) handleElicitationResponse(action tools.ElicitationAction, content map[string]any, elicitationID string) (tea.Model, tea.Cmd) { + if err := m.application.ResumeElicitation(m.ctx(), action, content, elicitationID); err != nil { slog.Error("Failed to resume elicitation", "action", action, "error", err) return m, notification.ErrorCmd("Failed to complete server request: " + err.Error()) } diff --git a/pkg/tui/messages/mcp.go b/pkg/tui/messages/mcp.go index 3d18624a06..231c5bd20d 100644 --- a/pkg/tui/messages/mcp.go +++ b/pkg/tui/messages/mcp.go @@ -20,5 +20,9 @@ type ( ElicitationResponseMsg struct { Action tools.ElicitationAction Content map[string]any + // ElicitationID correlates this response with the specific request it + // answers; empty for dialogs built before the ID was plumbed through + // (falls back to the runtime's sole-pending-request behavior). + ElicitationID string } ) diff --git a/pkg/tui/page/chat/runtime_events.go b/pkg/tui/page/chat/runtime_events.go index 1d77319ac3..60835fd887 100644 --- a/pkg/tui/page/chat/runtime_events.go +++ b/pkg/tui/page/chat/runtime_events.go @@ -449,7 +449,7 @@ func (p *chatPage) handleElicitationRequest(msg *runtime.ElicitationRequestEvent serverURL = url } dialogCmd := core.CmdHandler(dialog.OpenDialogMsg{ - Model: dialog.NewOAuthAuthorizationDialog(p.ctx(), serverURL, p.app), + Model: dialog.NewOAuthAuthorizationDialog(p.ctx(), serverURL, msg.ElicitationID, p.app), OriginatingEvent: msg, }) return tea.Batch(spinnerCmd, dialogCmd) @@ -461,7 +461,7 @@ func (p *chatPage) handleElicitationRequest(msg *runtime.ElicitationRequestEvent case "url": // URL-based elicitation - show URL dialog dialogCmd := core.CmdHandler(dialog.OpenDialogMsg{ - Model: dialog.NewURLElicitationDialog(p.ctx(), msg.Message, msg.URL), + Model: dialog.NewURLElicitationDialog(p.ctx(), msg.Message, msg.URL, msg.ElicitationID), OriginatingEvent: msg, }) return tea.Batch(spinnerCmd, dialogCmd) @@ -469,7 +469,7 @@ func (p *chatPage) handleElicitationRequest(msg *runtime.ElicitationRequestEvent default: // Form-based elicitation (default) - show form dialog dialogCmd := core.CmdHandler(dialog.OpenDialogMsg{ - Model: dialog.NewElicitationDialog(msg.Message, msg.Schema, msg.Meta), + Model: dialog.NewElicitationDialog(msg.Message, msg.Schema, msg.Meta, msg.ElicitationID), OriginatingEvent: msg, }) return tea.Batch(spinnerCmd, dialogCmd) diff --git a/pkg/tui/service/supervisor/supervisor.go b/pkg/tui/service/supervisor/supervisor.go index 2286c05d5d..16b0e5ce79 100644 --- a/pkg/tui/service/supervisor/supervisor.go +++ b/pkg/tui/service/supervisor/supervisor.go @@ -185,15 +185,28 @@ func (s *Supervisor) handleRuntimeEvent(sessionID string, msg tea.Msg) { case *runtime.StreamStartedEvent: if isTopLevelStream(runner.ID, ev.SessionID) { runner.IsRunning = true - runner.PendingEvents = nil // New top-level stream supersedes any stale pending events + // A new top-level turn supersedes any stale attention events raised + // by ITS OWN previous turn, but must not discard a still-live, + // unanswered elicitation from a detached background job + // (run_background_agent outlives the turn boundary via + // context.WithoutCancel) — that job's waiter goroutine is still + // blocked and would be orphaned if its prompt vanished from the + // queue (#3584 review item 4). + runner.PendingEvents = retainDetachedElicitations(runner.ID, runner.PendingEvents) + runner.NeedsAttn = len(runner.PendingEvents) > 0 s.notifyTabsUpdated() } case *runtime.StreamStoppedEvent: if isTopLevelStream(runner.ID, ev.SessionID) { runner.IsRunning = false - runner.PendingEvents = nil // Clear any pending attention events since the top-level stream ended - runner.NeedsAttn = false + // Same rule as StreamStarted above: only this runner's own + // top-level attention events are moot now that its stream ended. + // A detached background job's live elicitation must survive the + // foreground stream's stop so its waiter isn't orphaned (#3584 + // review item 4). + runner.PendingEvents = retainDetachedElicitations(runner.ID, runner.PendingEvents) + runner.NeedsAttn = len(runner.PendingEvents) > 0 s.notifyTabsUpdated() } @@ -215,6 +228,21 @@ func (s *Supervisor) handleRuntimeEvent(sessionID string, msg tea.Msg) { } } +// retainDetachedElicitations filters pending to keep only +// ElicitationRequestEvents raised by a session other than runnerID — i.e. a +// detached background job's sub-session, whose elicitation waiter is still +// blocked awaiting a response regardless of what the runner's own top-level +// stream is doing. Everything else (ToolCallConfirmation, MaxIterationsReached, +// and elicitations belonging to runnerID's own top-level stream) is dropped: +// those are inherently scoped to the stream that just started or stopped, so +// they are genuinely moot once it does. +func retainDetachedElicitations(runnerID string, pending []tea.Msg) []tea.Msg { + return slices.DeleteFunc(pending, func(msg tea.Msg) bool { + elic, ok := msg.(*runtime.ElicitationRequestEvent) + return !ok || isTopLevelStream(runnerID, elic.SessionID) + }) +} + // notifyTabsUpdated sends a tabs updated message (must be called with lock held). func (s *Supervisor) notifyTabsUpdated() { p := s.program diff --git a/pkg/tui/service/supervisor/supervisor_test.go b/pkg/tui/service/supervisor/supervisor_test.go index bc3376859c..80bcc9f207 100644 --- a/pkg/tui/service/supervisor/supervisor_test.go +++ b/pkg/tui/service/supervisor/supervisor_test.go @@ -324,3 +324,86 @@ func TestStreamStarted_EmptySessionID_TreatedAsTopLevel(t *testing.T) { "empty SessionID must be treated as top-level and supersede stale pending event") assert.True(t, s.runners["sess-A"].IsRunning) } + +// --- #3584 review item 4: foreground stream stop must not discard a +// still-live detached background job's elicitation --- + +// TestStreamStopped_TopLevel_PreservesDetachedBackgroundElicitation is the +// regression test for review item 4: a background job started via +// run_background_agent outlives its parent's top-level stream +// (context.WithoutCancel), so its own live, unanswered elicitation can still +// be queued on this runner when the FOREGROUND stream stops. The old +// unconditional `PendingEvents = nil` wiped it out from under the background +// job's still-blocked waiter goroutine. Only the runner's OWN top-level +// attention events (here, none) are moot; the background elicitation +// (SessionID "bg-child", distinct from the runner's own "sess-A") must +// survive. +func TestStreamStopped_TopLevel_PreservesDetachedBackgroundElicitation(t *testing.T) { + t.Parallel() + s := newTestSupervisor([]string{"sess-A"}, "sess-B") // sess-A inactive + + bgElicitation := runtime.ElicitationRequest("bg needs input", "form", nil, "", "eid-bg", "", "bg-child", nil, "agent") + s.runners["sess-A"].PendingEvents = []tea.Msg{bgElicitation} + s.runners["sess-A"].NeedsAttn = true + s.runners["sess-A"].IsRunning = true + + // The foreground (top-level) stream for sess-A stops. + s.handleRuntimeEvent("sess-A", &runtime.StreamStoppedEvent{ + Type: "stream_stopped", + SessionID: "sess-A", + }) + + assert.False(t, s.runners["sess-A"].IsRunning, "the foreground stream itself must still be marked stopped") + require.Equal(t, []tea.Msg{bgElicitation}, s.runners["sess-A"].PendingEvents, + "a detached background job's live elicitation must survive the foreground stream's stop") + assert.True(t, s.runners["sess-A"].NeedsAttn, + "NeedsAttn must stay true while a background elicitation is still queued") +} + +// TestStreamStopped_TopLevel_MixedPending_OnlyForegroundEventsCleared covers +// the more realistic mixed case: a foreground-owned tool confirmation and a +// background job's elicitation are both queued. Stopping the foreground +// stream must clear only the foreground-scoped entry. +func TestStreamStopped_TopLevel_MixedPending_OnlyForegroundEventsCleared(t *testing.T) { + t.Parallel() + s := newTestSupervisor([]string{"sess-A"}, "sess-B") + + foregroundConfirmation := runtime.ToolCallConfirmation(tools.ToolCall{}, tools.Tool{}, "agent", nil) + bgElicitation := runtime.ElicitationRequest("bg needs input", "form", nil, "", "eid-bg", "", "bg-child", nil, "agent") + s.runners["sess-A"].PendingEvents = []tea.Msg{foregroundConfirmation, bgElicitation} + s.runners["sess-A"].NeedsAttn = true + s.runners["sess-A"].IsRunning = true + + s.handleRuntimeEvent("sess-A", &runtime.StreamStoppedEvent{ + Type: "stream_stopped", + SessionID: "sess-A", + }) + + require.Equal(t, []tea.Msg{bgElicitation}, s.runners["sess-A"].PendingEvents, + "only the foreground-scoped tool confirmation must be dropped; the background elicitation stays queued") + assert.True(t, s.runners["sess-A"].NeedsAttn) +} + +// TestStreamStarted_TopLevel_PreservesDetachedBackgroundElicitation mirrors +// the StreamStopped case for a NEW top-level turn starting on the same tab +// while a background job from a previous turn is still live: starting a new +// foreground turn must not orphan the background job's queued elicitation +// either. +func TestStreamStarted_TopLevel_PreservesDetachedBackgroundElicitation(t *testing.T) { + t.Parallel() + s := newTestSupervisor([]string{"sess-A"}, "sess-B") + + bgElicitation := runtime.ElicitationRequest("bg needs input", "form", nil, "", "eid-bg2", "", "bg-child-2", nil, "agent") + s.runners["sess-A"].PendingEvents = []tea.Msg{bgElicitation} + s.runners["sess-A"].NeedsAttn = true + + s.handleRuntimeEvent("sess-A", &runtime.StreamStartedEvent{ + Type: "stream_started", + SessionID: "sess-A", + }) + + assert.True(t, s.runners["sess-A"].IsRunning) + require.Equal(t, []tea.Msg{bgElicitation}, s.runners["sess-A"].PendingEvents, + "a new top-level turn must not discard a still-live detached background elicitation") + assert.True(t, s.runners["sess-A"].NeedsAttn) +} diff --git a/pkg/tui/tui.go b/pkg/tui/tui.go index 5ca479ae21..c249306cfd 100644 --- a/pkg/tui/tui.go +++ b/pkg/tui/tui.go @@ -1309,7 +1309,7 @@ func (m *appModel) update(msg tea.Msg) (tea.Model, tea.Cmd) { // --- Elicitation --- case messages.ElicitationResponseMsg: - return m.handleElicitationResponse(msg.Action, msg.Content) + return m.handleElicitationResponse(msg.Action, msg.Content, msg.ElicitationID) // --- Errors --- @@ -1902,7 +1902,7 @@ func (m *appModel) replayElicitationEvent(ev *runtime.ElicitationRequestEvent) t serverURL = url } return core.CmdHandler(dialog.OpenDialogMsg{ - Model: dialog.NewOAuthAuthorizationDialog(m.ctx(), serverURL, m.application), + Model: dialog.NewOAuthAuthorizationDialog(m.ctx(), serverURL, ev.ElicitationID, m.application), OriginatingEvent: ev, }) } @@ -1911,12 +1911,12 @@ func (m *appModel) replayElicitationEvent(ev *runtime.ElicitationRequestEvent) t switch ev.Mode { case "url": return core.CmdHandler(dialog.OpenDialogMsg{ - Model: dialog.NewURLElicitationDialog(m.ctx(), ev.Message, ev.URL), + Model: dialog.NewURLElicitationDialog(m.ctx(), ev.Message, ev.URL, ev.ElicitationID), OriginatingEvent: ev, }) default: return core.CmdHandler(dialog.OpenDialogMsg{ - Model: dialog.NewElicitationDialog(ev.Message, ev.Schema, ev.Meta), + Model: dialog.NewElicitationDialog(ev.Message, ev.Schema, ev.Meta, ev.ElicitationID), OriginatingEvent: ev, }) } From 608cfe80a946cd3724f420ce6e346454a45b7a11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arnaud=20He=CC=81ritier?= Date: Mon, 13 Jul 2026 15:07:24 +0200 Subject: [PATCH 2/5] fix(app): stop delivering foreground elicitations twice into a.events (#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. --- pkg/app/app.go | 46 ++++++++++++++++++++++++--- pkg/app/app_test.go | 76 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 4 deletions(-) diff --git a/pkg/app/app.go b/pkg/app/app.go index fd1860eb65..f6cf640abe 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -199,10 +199,12 @@ func (a *App) Start(ctx context.Context) { // including background-job (run_background_agent) sub-sessions whose // RunStream has no live UI reading its own events channel — so they // always reach the TUI as a dialog. This is the runtime's single, - // exactly-once delivery point for elicitation requests (#3584); the - // swap-based bridge is a separate best-effort path for remote/SSE - // consumers only and never also reaches this sink, so no App-side - // dedupe is needed here. + // exactly-once delivery point for elicitation requests (#3584). For a + // foreground request the swap-based bridge best-effort-sends the same + // event on the very RunStream channel Run/Retry/RunWithMessage read + // from below; those loops skip ElicitationRequestEvents so this sink + // stays the sole route into a.events and a foreground request never + // opens two dialogs (#3584 review). a.runtime.OnElicitationRequest(func(event runtime.Event) { a.sendEvent(ctx, event) }) @@ -524,6 +526,13 @@ func (a *App) Run(ctx context.Context, cancel context.CancelFunc, message string continue } + // Already delivered via the OnElicitationRequest sink (Start); skip + // the bridge's secondary copy on this channel to avoid a second + // dialog for the same request (#3584). + if isElicitationRequestEvent(event) { + continue + } + // Clear titleGenerating flag when title is generated (from server for remote runtime) if _, ok := event.(*runtime.SessionTitleEvent); ok { a.titleGenerating.Store(false) @@ -697,6 +706,21 @@ func (a *App) sendEvent(ctx context.Context, event tea.Msg) { } } +// isElicitationRequestEvent reports whether event is an +// *runtime.ElicitationRequestEvent. Start registers an OnElicitationRequest +// sink that delivers every elicitation request into a.events synchronously +// and exactly once (elicitationHandler, #3584). For a foreground request +// that sink delivery and the best-effort bridge secondary delivery land on +// the very same RunStream channel these loops read from, so any loop that +// forwards RunStream events into a.events verbatim must skip this event type +// to avoid a second dialog for the same request — mirroring the exclusion +// agent_delegation.go's runCollecting already applies for background +// sub-sessions sharing the bridge slot. +func isElicitationRequestEvent(event runtime.Event) bool { + _, ok := event.(*runtime.ElicitationRequestEvent) + return ok +} + // acquireStreamGuard locks a.streamGuard (set via WithStreamGuard) and // returns the matching release func, or a no-op release when no guard is // attached (a bare App with no SessionManager to race against). Callers must @@ -752,6 +776,13 @@ func (a *App) Retry(ctx context.Context, cancel context.CancelFunc) { continue } + // Already delivered via the OnElicitationRequest sink (Start); skip + // the bridge's secondary copy on this channel to avoid a second + // dialog for the same request (#3584). + if isElicitationRequestEvent(event) { + continue + } + switch event.(type) { case *runtime.StreamStartedEvent: streamStarted = true @@ -809,6 +840,13 @@ func (a *App) RunWithMessage(ctx context.Context, cancel context.CancelFunc, msg continue } + // Already delivered via the OnElicitationRequest sink (Start); skip + // the bridge's secondary copy on this channel to avoid a second + // dialog for the same request (#3584). + if isElicitationRequestEvent(event) { + continue + } + // Clear titleGenerating flag when title is generated (from server for remote runtime) if _, ok := event.(*runtime.SessionTitleEvent); ok { a.titleGenerating.Store(false) diff --git a/pkg/app/app_test.go b/pkg/app/app_test.go index 1ac9329c74..b7a48fddc0 100644 --- a/pkg/app/app_test.go +++ b/pkg/app/app_test.go @@ -917,3 +917,79 @@ func TestApp_CompactLiveSession_ForwardsRuntimeError(t *testing.T) { err := app.CompactLiveSession(t.Context(), "x", "") require.ErrorContains(t, err, "not live") } + +// composedElicitationMockRuntime reproduces the production wiring that +// exists once both the OnElicitationRequest sink (registered by App.Start) +// and a live RunStream (read directly by App.Run/Retry/RunWithMessage) are +// present for the same foreground request. LocalRuntime.elicitationHandler +// delivers a request to the sink synchronously and exactly once, then +// separately best-effort-sends the same event on the elicitation bridge — +// which, for a foreground stream, is the very channel RunStream returns. +// This mock drives both paths the same way so tests can assert on the +// combined result App actually observes (#3584). +type composedElicitationMockRuntime struct { + mockRuntime + + handler func(runtime.Event) +} + +func (m *composedElicitationMockRuntime) OnElicitationRequest(handler func(runtime.Event)) { + m.handler = handler +} + +func (m *composedElicitationMockRuntime) RunStream(_ context.Context, sess *session.Session) <-chan runtime.Event { + ch := make(chan runtime.Event, 8) + go func() { + defer close(ch) + ch <- runtime.StreamStarted(sess.ID, "mock") + + ev := runtime.ElicitationRequest("need input", "form", nil, "", "eid-1", "", sess.ID, nil, "mock") + if m.handler != nil { + m.handler(ev) // reliable sink delivery (elicitationHandler, #3584) + } + ch <- ev // best-effort bridge delivery on the same RunStream channel + + ch <- runtime.StreamStopped(sess.ID, "mock", "normal") + }() + return ch +} + +// TestReview_ForegroundElicitationIsNotDeliveredTwice is the reviewer's +// regression probe for the #3584 double-delivery bug: a foreground +// LocalRuntime elicitation request reached a.events via BOTH the +// OnElicitationRequest sink (Start) and the RunStream forwarding loop +// (Run), producing two dialogs for one request. Exactly one +// ElicitationRequestEvent must reach the app's event stream per request. +func TestReview_ForegroundElicitationIsNotDeliveredTwice(t *testing.T) { + t.Parallel() + + rt := &composedElicitationMockRuntime{} + events := make(chan tea.Msg, 16) + app := &App{runtime: rt, session: session.New(), events: events} + + app.Start(t.Context()) + require.NotNil(t, rt.handler, "Start must register the OnElicitationRequest handler") + + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + app.Run(ctx, cancel, "hello", nil) + + elicitations := 0 + deadline := time.After(2 * time.Second) +drain: + for { + select { + case msg := <-events: + if _, ok := msg.(*runtime.ElicitationRequestEvent); ok { + elicitations++ + } + if _, ok := msg.(*runtime.StreamStoppedEvent); ok { + break drain + } + case <-deadline: + t.Fatal("timed out waiting for the stream to stop") + } + } + + assert.Equal(t, 1, elicitations, "a single foreground elicitation must open exactly one dialog") +} From a7f905ad115c52a12d49f31dcf2259ad7d377ff7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arnaud=20He=CC=81ritier?= Date: Mon, 13 Jul 2026 15:38:37 +0200 Subject: [PATCH 3/5] fix(app): only skip RunStream elicitation events when the sink already delivered them (#3584) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous fix (2ad095ae0) 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 2ad095ae0'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). --- pkg/app/app.go | 87 ++++++++++++++++++++++++----------- pkg/app/app_test.go | 70 ++++++++++++++++++++++++++++ pkg/runtime/elicitation.go | 17 +++++++ pkg/runtime/remote_runtime.go | 6 +++ 4 files changed, 152 insertions(+), 28 deletions(-) diff --git a/pkg/app/app.go b/pkg/app/app.go index f6cf640abe..b487d87344 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -198,13 +198,15 @@ func (a *App) Start(ctx context.Context) { // Forward elicitation requests raised anywhere in the runtime — // including background-job (run_background_agent) sub-sessions whose // RunStream has no live UI reading its own events channel — so they - // always reach the TUI as a dialog. This is the runtime's single, - // exactly-once delivery point for elicitation requests (#3584). For a - // foreground request the swap-based bridge best-effort-sends the same - // event on the very RunStream channel Run/Retry/RunWithMessage read - // from below; those loops skip ElicitationRequestEvents so this sink - // stays the sole route into a.events and a foreground request never - // opens two dialogs (#3584 review). + // always reach the TUI as a dialog. For runtimes that mirror sink + // deliveries onto their RunStream channel too (LocalRuntime: the + // swap-based bridge best-effort-sends the same event on the very + // RunStream channel Run/Retry/RunWithMessage read from below), this + // sink is the single, exactly-once delivery point and those loops skip + // the mirrored copy (see mustSkipMirroredElicitation). Runtimes that + // don't mirror it (RemoteRuntime, whose OnElicitationRequest below is + // a no-op) deliver elicitations only through that RunStream copy, + // which those loops forward unfiltered (#3584 review). a.runtime.OnElicitationRequest(func(event runtime.Event) { a.sendEvent(ctx, event) }) @@ -513,6 +515,7 @@ func (a *App) Run(ctx context.Context, cancel context.CancelFunc, message string } else { a.session.AddMessage(session.UserMessage(message)) } + skipMirroredElicitation := mustSkipMirroredElicitation(a.runtime) for event := range a.runtime.RunStream(ctx, a.session) { // If context is cancelled, continue draining but don't forward events // — except StreamStoppedEvent, which must always propagate so the @@ -526,10 +529,12 @@ func (a *App) Run(ctx context.Context, cancel context.CancelFunc, message string continue } - // Already delivered via the OnElicitationRequest sink (Start); skip - // the bridge's secondary copy on this channel to avoid a second - // dialog for the same request (#3584). - if isElicitationRequestEvent(event) { + // Already delivered via the OnElicitationRequest sink (Start) for + // runtimes that mirror it onto RunStream too; skip that duplicate + // copy to avoid a second dialog for the same request. Runtimes that + // deliver ONLY via RunStream (e.g. RemoteRuntime) are unaffected — + // mustSkipMirroredElicitation is false for them (#3584 review). + if skipMirroredElicitation && isElicitationRequestEvent(event) { continue } @@ -707,20 +712,40 @@ func (a *App) sendEvent(ctx context.Context, event tea.Msg) { } // isElicitationRequestEvent reports whether event is an -// *runtime.ElicitationRequestEvent. Start registers an OnElicitationRequest -// sink that delivers every elicitation request into a.events synchronously -// and exactly once (elicitationHandler, #3584). For a foreground request -// that sink delivery and the best-effort bridge secondary delivery land on -// the very same RunStream channel these loops read from, so any loop that -// forwards RunStream events into a.events verbatim must skip this event type -// to avoid a second dialog for the same request — mirroring the exclusion -// agent_delegation.go's runCollecting already applies for background -// sub-sessions sharing the bridge slot. +// *runtime.ElicitationRequestEvent. func isElicitationRequestEvent(event runtime.Event) bool { _, ok := event.(*runtime.ElicitationRequestEvent) return ok } +// elicitationSinkMirror is an optional runtime capability satisfied by +// runtimes whose OnElicitationRequest sink is the single, exactly-once +// delivery point for elicitation requests even though the runtime ALSO +// best-effort-mirrors the same event onto its RunStream channel, for the +// benefit of out-of-process consumers reading RunStream directly (see +// runtime.LocalRuntime.MirrorsElicitationOnRunStream and elicitationHandler, +// #3584). +type elicitationSinkMirror interface { + MirrorsElicitationOnRunStream() +} + +// mustSkipMirroredElicitation reports whether rt implements +// elicitationSinkMirror, i.e. whether Start's OnElicitationRequest sink +// (below) already delivers every elicitation from rt exactly once, making +// any *runtime.ElicitationRequestEvent read directly off RunStream a +// duplicate that Run/Retry/RunWithMessage must skip to avoid a second +// dialog for the same request. +// +// Runtimes that do NOT implement it (RemoteRuntime, whose +// OnElicitationRequest is a documented no-op) deliver elicitations ONLY via +// RunStream, so that copy is the sole delivery and must reach a.events +// unfiltered — an earlier fix skipped this event unconditionally and +// silently dropped every remote elicitation as a result (#3584 review). +func mustSkipMirroredElicitation(rt runtime.Runtime) bool { + _, ok := rt.(elicitationSinkMirror) + return ok +} + // acquireStreamGuard locks a.streamGuard (set via WithStreamGuard) and // returns the matching release func, or a no-op release when no guard is // attached (a bare App with no SessionManager to race against). Callers must @@ -765,6 +790,7 @@ func (a *App) Retry(ctx context.Context, cancel context.CancelFunc) { defer release() streamStarted := false + skipMirroredElicitation := mustSkipMirroredElicitation(a.runtime) for event := range a.runtime.RunStream(ctx, a.session) { // If context is cancelled, continue draining but don't forward events // — except StreamStoppedEvent, which must always propagate so the @@ -776,10 +802,12 @@ func (a *App) Retry(ctx context.Context, cancel context.CancelFunc) { continue } - // Already delivered via the OnElicitationRequest sink (Start); skip - // the bridge's secondary copy on this channel to avoid a second - // dialog for the same request (#3584). - if isElicitationRequestEvent(event) { + // Already delivered via the OnElicitationRequest sink (Start) for + // runtimes that mirror it onto RunStream too; skip that duplicate + // copy to avoid a second dialog for the same request. Runtimes that + // deliver ONLY via RunStream (e.g. RemoteRuntime) are unaffected — + // mustSkipMirroredElicitation is false for them (#3584 review). + if skipMirroredElicitation && isElicitationRequestEvent(event) { continue } @@ -827,6 +855,7 @@ func (a *App) RunWithMessage(ctx context.Context, cancel context.CancelFunc, msg defer release() a.session.AddMessage(msg) + skipMirroredElicitation := mustSkipMirroredElicitation(a.runtime) for event := range a.runtime.RunStream(ctx, a.session) { // If context is cancelled, continue draining but don't forward events // — except StreamStoppedEvent, which must always propagate so the @@ -840,10 +869,12 @@ func (a *App) RunWithMessage(ctx context.Context, cancel context.CancelFunc, msg continue } - // Already delivered via the OnElicitationRequest sink (Start); skip - // the bridge's secondary copy on this channel to avoid a second - // dialog for the same request (#3584). - if isElicitationRequestEvent(event) { + // Already delivered via the OnElicitationRequest sink (Start) for + // runtimes that mirror it onto RunStream too; skip that duplicate + // copy to avoid a second dialog for the same request. Runtimes that + // deliver ONLY via RunStream (e.g. RemoteRuntime) are unaffected — + // mustSkipMirroredElicitation is false for them (#3584 review). + if skipMirroredElicitation && isElicitationRequestEvent(event) { continue } diff --git a/pkg/app/app_test.go b/pkg/app/app_test.go index b7a48fddc0..2ca90bb98a 100644 --- a/pkg/app/app_test.go +++ b/pkg/app/app_test.go @@ -937,6 +937,12 @@ func (m *composedElicitationMockRuntime) OnElicitationRequest(handler func(runti m.handler = handler } +// MirrorsElicitationOnRunStream marks this mock as reproducing LocalRuntime's +// mirrored-delivery contract (runtime.LocalRuntime.MirrorsElicitationOnRunStream), +// so App's RunStream-forwarding loops must skip the duplicate copy this mock +// also pushes onto RunStream below. +func (m *composedElicitationMockRuntime) MirrorsElicitationOnRunStream() {} + func (m *composedElicitationMockRuntime) RunStream(_ context.Context, sess *session.Session) <-chan runtime.Event { ch := make(chan runtime.Event, 8) go func() { @@ -954,6 +960,70 @@ func (m *composedElicitationMockRuntime) RunStream(_ context.Context, sess *sess return ch } +// remoteLikeMockRuntime mirrors RemoteRuntime's elicitation contract: its +// OnElicitationRequest sink is a no-op (mockRuntime's default; see +// pkg/runtime/remote_runtime.go's OnElicitationRequest) and every +// elicitation request arrives ONLY as a *runtime.ElicitationRequestEvent on +// the RunStream channel, exactly like remote_runtime.go's RunStream +// forwarding loop. It deliberately does NOT implement +// elicitationSinkMirror, so App must not skip this event for it. +type remoteLikeMockRuntime struct { + mockRuntime +} + +func (m *remoteLikeMockRuntime) RunStream(_ context.Context, sess *session.Session) <-chan runtime.Event { + ch := make(chan runtime.Event, 8) + go func() { + defer close(ch) + ch <- runtime.StreamStarted(sess.ID, "mock") + ch <- runtime.ElicitationRequest("need input", "form", nil, "", "eid-1", "", sess.ID, nil, "mock") + ch <- runtime.StreamStopped(sess.ID, "mock", "normal") + }() + return ch +} + +// TestReview_RemoteRuntimeElicitationIsStillDelivered is the regression probe +// for the over-broad fix to the #3584 double-delivery bug: commit 2ad095ae0 +// made App.Run/Retry/RunWithMessage skip every +// *runtime.ElicitationRequestEvent read off RunStream UNCONDITIONALLY, which +// also silently dropped elicitations for runtimes whose OnElicitationRequest +// sink is a no-op and that deliver ONLY via RunStream (RemoteRuntime) — a +// remote-backed session showed zero dialogs instead of one. A runtime that +// does not mirror sink deliveries onto RunStream must have that event pass +// through to a.events untouched. +func TestReview_RemoteRuntimeElicitationIsStillDelivered(t *testing.T) { + t.Parallel() + + rt := &remoteLikeMockRuntime{} + events := make(chan tea.Msg, 16) + app := &App{runtime: rt, session: session.New(), events: events} + + app.Start(t.Context()) + + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + app.Run(ctx, cancel, "hello", nil) + + elicitations := 0 + deadline := time.After(2 * time.Second) +drain: + for { + select { + case msg := <-events: + if _, ok := msg.(*runtime.ElicitationRequestEvent); ok { + elicitations++ + } + if _, ok := msg.(*runtime.StreamStoppedEvent); ok { + break drain + } + case <-deadline: + t.Fatal("timed out waiting for the stream to stop") + } + } + + assert.Equal(t, 1, elicitations, "a remote/no-sink runtime's elicitation must still reach the app via RunStream") +} + // TestReview_ForegroundElicitationIsNotDeliveredTwice is the reviewer's // regression probe for the #3584 double-delivery bug: a foreground // LocalRuntime elicitation request reached a.events via BOTH the diff --git a/pkg/runtime/elicitation.go b/pkg/runtime/elicitation.go index fdc3468635..666c75d07b 100644 --- a/pkg/runtime/elicitation.go +++ b/pkg/runtime/elicitation.go @@ -356,6 +356,23 @@ func (r *LocalRuntime) OnElicitationRequest(handler func(Event)) { r.onElicitationRequest = handler } +// MirrorsElicitationOnRunStream marks LocalRuntime as a runtime whose +// OnElicitationRequest sink is the single, exactly-once delivery point for +// an elicitation request even though elicitationHandler ALSO best-effort- +// sends the very same event on the RunStream channel, for the benefit of +// out-of-process consumers reading RunStream directly (see +// elicitationBridge). Embedders that forward RunStream events verbatim into +// their own event bus (e.g. pkg/app.App) use this marker — via an optional +// capability check, since it is not part of the Runtime interface — to know +// they must skip *ElicitationRequestEvent to avoid delivering the same +// request twice. +// +// RemoteRuntime deliberately does NOT implement this: its +// OnElicitationRequest is a no-op, so the copy on its RunStream is the ONLY +// delivery and callers must not skip it (#3584 review — an earlier fix +// skipped unconditionally and silently dropped every remote elicitation). +func (r *LocalRuntime) MirrorsElicitationOnRunStream() {} + // emitElicitationRequest forwards an elicitation request event to the // registered sink, if any. Besides [LocalRuntime.EmitElicitationRequestForTesting], // this is the ONLY call site that invokes the sink (see elicitationHandler): diff --git a/pkg/runtime/remote_runtime.go b/pkg/runtime/remote_runtime.go index ce22198141..3145097e8b 100644 --- a/pkg/runtime/remote_runtime.go +++ b/pkg/runtime/remote_runtime.go @@ -711,6 +711,12 @@ func (r *RemoteRuntime) OnBackgroundEvent(func(Event)) {} // (including from server-side background jobs) arrive as ElicitationRequestEvent // values on the RunStream channel itself (see the RunStream forwarding loop // above), so there is no separate out-of-band sink to register. +// +// RemoteRuntime deliberately does NOT implement +// LocalRuntime.MirrorsElicitationOnRunStream: embedders that forward +// RunStream events verbatim (e.g. pkg/app.App) rely on that capability check +// to tell that this runtime's RunStream copy is its ONLY delivery and must +// reach them unfiltered (#3584 review). func (r *RemoteRuntime) OnElicitationRequest(func(Event)) {} // Close is a no-op for remote runtimes. From 44abc9009772790dcf4fc72274cc30a8d2341250 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arnaud=20He=CC=81ritier?= Date: Mon, 13 Jul 2026 16:16:18 +0200 Subject: [PATCH 4/5] test(app): harden double-delivery regression coverage and pin concrete-runtime classification (#3584) Review of bff856078 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). --- pkg/app/app.go | 145 ++++++++++++++++------------------------ pkg/app/app_test.go | 158 ++++++++++++++++++++++++++++++++++---------- 2 files changed, 179 insertions(+), 124 deletions(-) diff --git a/pkg/app/app.go b/pkg/app/app.go index b487d87344..d488d76a17 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -515,36 +515,7 @@ func (a *App) Run(ctx context.Context, cancel context.CancelFunc, message string } else { a.session.AddMessage(session.UserMessage(message)) } - skipMirroredElicitation := mustSkipMirroredElicitation(a.runtime) - for event := range a.runtime.RunStream(ctx, a.session) { - // If context is cancelled, continue draining but don't forward events - // — except StreamStoppedEvent, which must always propagate so the - // supervisor can mark the session as no longer running. - if ctx.Err() != nil { - if _, ok := event.(*runtime.StreamStoppedEvent); ok { - // ctx is cancelled; detach cancellation but keep its trace - // context so the stop event still reaches subscribers. - a.sendEvent(context.WithoutCancel(ctx), event) - } - continue - } - - // Already delivered via the OnElicitationRequest sink (Start) for - // runtimes that mirror it onto RunStream too; skip that duplicate - // copy to avoid a second dialog for the same request. Runtimes that - // deliver ONLY via RunStream (e.g. RemoteRuntime) are unaffected — - // mustSkipMirroredElicitation is false for them (#3584 review). - if skipMirroredElicitation && isElicitationRequestEvent(event) { - continue - } - - // Clear titleGenerating flag when title is generated (from server for remote runtime) - if _, ok := event.(*runtime.SessionTitleEvent); ok { - a.titleGenerating.Store(false) - } - - a.sendEvent(ctx, event) - } + a.forwardRunStreamEvents(ctx, a.runtime.RunStream(ctx, a.session), nil) }() } @@ -746,6 +717,56 @@ func mustSkipMirroredElicitation(rt runtime.Runtime) bool { return ok } +// forwardRunStreamEvents drains ch and forwards each event to a.events, +// applying the bookkeeping shared by Run, Retry, and RunWithMessage. This is +// the SINGLE place that decides whether a RunStream event reaches a.events: +// centralizing it here (instead of three copies of the same loop) is what +// lets one set of tests cover all three entry points, and makes it +// impossible for one of them to silently regress independently of the +// others (#3584 review — Retry and RunWithMessage had duplicated this exact +// logic untested). +// +// filter, when non-nil, runs after the elicitation-mirroring skip below and +// may itself veto forwarding an event by returning false. Retry uses it to +// suppress the pre-StreamStarted re-emitted user message; Run and +// RunWithMessage pass nil. +func (a *App) forwardRunStreamEvents(ctx context.Context, ch <-chan runtime.Event, filter func(event runtime.Event) (forward bool)) { + skipMirroredElicitation := mustSkipMirroredElicitation(a.runtime) + for event := range ch { + // If context is cancelled, continue draining but don't forward events + // — except StreamStoppedEvent, which must always propagate so the + // supervisor can mark the session as no longer running. + if ctx.Err() != nil { + if _, ok := event.(*runtime.StreamStoppedEvent); ok { + // ctx is cancelled; detach cancellation but keep its trace + // context so the stop event still reaches subscribers. + a.sendEvent(context.WithoutCancel(ctx), event) + } + continue + } + + // Already delivered via the OnElicitationRequest sink (Start) for + // runtimes that mirror it onto RunStream too; skip that duplicate + // copy to avoid a second dialog for the same request. Runtimes that + // deliver ONLY via RunStream (e.g. RemoteRuntime) are unaffected — + // mustSkipMirroredElicitation is false for them (#3584 review). + if skipMirroredElicitation && isElicitationRequestEvent(event) { + continue + } + + if filter != nil && !filter(event) { + continue + } + + // Clear titleGenerating flag when title is generated (from server for remote runtime) + if _, ok := event.(*runtime.SessionTitleEvent); ok { + a.titleGenerating.Store(false) + } + + a.sendEvent(ctx, event) + } +} + // acquireStreamGuard locks a.streamGuard (set via WithStreamGuard) and // returns the matching release func, or a no-op release when no guard is // attached (a bare App with no SessionManager to race against). Callers must @@ -790,42 +811,17 @@ func (a *App) Retry(ctx context.Context, cancel context.CancelFunc) { defer release() streamStarted := false - skipMirroredElicitation := mustSkipMirroredElicitation(a.runtime) - for event := range a.runtime.RunStream(ctx, a.session) { - // If context is cancelled, continue draining but don't forward events - // — except StreamStoppedEvent, which must always propagate so the - // supervisor can mark the session as no longer running. - if ctx.Err() != nil { - if _, ok := event.(*runtime.StreamStoppedEvent); ok { - a.sendEvent(context.WithoutCancel(ctx), event) - } - continue - } - - // Already delivered via the OnElicitationRequest sink (Start) for - // runtimes that mirror it onto RunStream too; skip that duplicate - // copy to avoid a second dialog for the same request. Runtimes that - // deliver ONLY via RunStream (e.g. RemoteRuntime) are unaffected — - // mustSkipMirroredElicitation is false for them (#3584 review). - if skipMirroredElicitation && isElicitationRequestEvent(event) { - continue - } - + a.forwardRunStreamEvents(ctx, a.runtime.RunStream(ctx, a.session), func(event runtime.Event) bool { switch event.(type) { case *runtime.StreamStartedEvent: streamStarted = true case *runtime.UserMessageEvent: if !streamStarted { - continue + return false } } - - if _, ok := event.(*runtime.SessionTitleEvent); ok { - a.titleGenerating.Store(false) - } - - a.sendEvent(ctx, event) - } + return true + }) }() } @@ -855,36 +851,7 @@ func (a *App) RunWithMessage(ctx context.Context, cancel context.CancelFunc, msg defer release() a.session.AddMessage(msg) - skipMirroredElicitation := mustSkipMirroredElicitation(a.runtime) - for event := range a.runtime.RunStream(ctx, a.session) { - // If context is cancelled, continue draining but don't forward events - // — except StreamStoppedEvent, which must always propagate so the - // supervisor can mark the session as no longer running. - if ctx.Err() != nil { - if _, ok := event.(*runtime.StreamStoppedEvent); ok { - // ctx is cancelled; detach cancellation but keep its trace - // context so the stop event still reaches subscribers. - a.sendEvent(context.WithoutCancel(ctx), event) - } - continue - } - - // Already delivered via the OnElicitationRequest sink (Start) for - // runtimes that mirror it onto RunStream too; skip that duplicate - // copy to avoid a second dialog for the same request. Runtimes that - // deliver ONLY via RunStream (e.g. RemoteRuntime) are unaffected — - // mustSkipMirroredElicitation is false for them (#3584 review). - if skipMirroredElicitation && isElicitationRequestEvent(event) { - continue - } - - // Clear titleGenerating flag when title is generated (from server for remote runtime) - if _, ok := event.(*runtime.SessionTitleEvent); ok { - a.titleGenerating.Store(false) - } - - a.sendEvent(ctx, event) - } + a.forwardRunStreamEvents(ctx, a.runtime.RunStream(ctx, a.session), nil) }() } diff --git a/pkg/app/app_test.go b/pkg/app/app_test.go index 2ca90bb98a..5811bc579f 100644 --- a/pkg/app/app_test.go +++ b/pkg/app/app_test.go @@ -982,6 +982,32 @@ func (m *remoteLikeMockRuntime) RunStream(_ context.Context, sess *session.Sessi return ch } +// countElicitationDeliveries drains events until a *runtime.StreamStoppedEvent +// (or a 2s timeout) and returns how many *runtime.ElicitationRequestEvent +// values were observed along the way. Shared by every regression test below +// so the drain/timeout logic isn't re-authored (and potentially +// mis-authored) per entry point. +func countElicitationDeliveries(t *testing.T, events <-chan tea.Msg) int { + t.Helper() + + n := 0 + deadline := time.After(2 * time.Second) + for { + select { + case msg := <-events: + if _, ok := msg.(*runtime.ElicitationRequestEvent); ok { + n++ + } + if _, ok := msg.(*runtime.StreamStoppedEvent); ok { + return n + } + case <-deadline: + t.Fatal("timed out waiting for the stream to stop") + return n + } + } +} + // TestReview_RemoteRuntimeElicitationIsStillDelivered is the regression probe // for the over-broad fix to the #3584 double-delivery bug: commit 2ad095ae0 // made App.Run/Retry/RunWithMessage skip every @@ -1004,24 +1030,7 @@ func TestReview_RemoteRuntimeElicitationIsStillDelivered(t *testing.T) { defer cancel() app.Run(ctx, cancel, "hello", nil) - elicitations := 0 - deadline := time.After(2 * time.Second) -drain: - for { - select { - case msg := <-events: - if _, ok := msg.(*runtime.ElicitationRequestEvent); ok { - elicitations++ - } - if _, ok := msg.(*runtime.StreamStoppedEvent); ok { - break drain - } - case <-deadline: - t.Fatal("timed out waiting for the stream to stop") - } - } - - assert.Equal(t, 1, elicitations, "a remote/no-sink runtime's elicitation must still reach the app via RunStream") + assert.Equal(t, 1, countElicitationDeliveries(t, events), "a remote/no-sink runtime's elicitation must still reach the app via RunStream") } // TestReview_ForegroundElicitationIsNotDeliveredTwice is the reviewer's @@ -1044,22 +1053,101 @@ func TestReview_ForegroundElicitationIsNotDeliveredTwice(t *testing.T) { defer cancel() app.Run(ctx, cancel, "hello", nil) - elicitations := 0 - deadline := time.After(2 * time.Second) -drain: - for { - select { - case msg := <-events: - if _, ok := msg.(*runtime.ElicitationRequestEvent); ok { - elicitations++ - } - if _, ok := msg.(*runtime.StreamStoppedEvent); ok { - break drain - } - case <-deadline: - t.Fatal("timed out waiting for the stream to stop") - } - } + assert.Equal(t, 1, countElicitationDeliveries(t, events), "a single foreground elicitation must open exactly one dialog") +} + +// TestMustSkipMirroredElicitation_ConcreteRuntimeClassification pins +// mustSkipMirroredElicitation's classification of the two CONCRETE +// production runtimes, not just marker-shaped mocks. The composed/remote +// regression tests above exercise independent mock types that each +// hand-declare whether they implement elicitationSinkMirror; they never +// touch runtime.LocalRuntime or runtime.RemoteRuntime, so an inverted +// production mapping (marker removed from LocalRuntime, or added to +// RemoteRuntime) would leave every other test in this file green (#3584 +// review — mutation testing proof). *runtime.LocalRuntime and +// *runtime.RemoteRuntime are asserted directly here; a nil pointer of each +// concrete type is enough since mustSkipMirroredElicitation only performs a +// type assertion and never calls a method on rt. +func TestMustSkipMirroredElicitation_ConcreteRuntimeClassification(t *testing.T) { + t.Parallel() + + assert.True(t, mustSkipMirroredElicitation((*runtime.LocalRuntime)(nil)), + "LocalRuntime mirrors OnElicitationRequest sink deliveries onto RunStream; App must skip the duplicate RunStream copy") + assert.False(t, mustSkipMirroredElicitation((*runtime.RemoteRuntime)(nil)), + "RemoteRuntime delivers elicitations ONLY via RunStream (its OnElicitationRequest sink is a no-op); App must not skip that copy") +} + +// elicitationEntryPoint names one of App's three RunStream-forwarding entry +// points and how to invoke it, so the delivery coverage below can drive all +// three through one table instead of duplicating the drive/drain logic. +type elicitationEntryPoint struct { + name string + invoke func(app *App, ctx context.Context, cancel context.CancelFunc) +} + +var elicitationEntryPoints = []elicitationEntryPoint{ + {"Run", func(app *App, ctx context.Context, cancel context.CancelFunc) { + app.Run(ctx, cancel, "hello", nil) + }}, + {"Retry", func(app *App, ctx context.Context, cancel context.CancelFunc) { + app.Retry(ctx, cancel) + }}, + {"RunWithMessage", func(app *App, ctx context.Context, cancel context.CancelFunc) { + app.RunWithMessage(ctx, cancel, session.UserMessage("hello")) + }}, +} + +// TestElicitationDeliveryAcrossEntryPoints is the mutation-hardened +// regression coverage for the #3584 double-delivery fix across ALL THREE +// RunStream-forwarding entry points. Before forwardRunStreamEvents +// centralized the gating logic, Run, Retry, and RunWithMessage each carried +// their own independent copy of it — only Run was ever driven through a +// mirrored and an unmirrored runtime, so breaking delivery in Retry (e.g. +// dropping the remote/unmirrored copy) or RunWithMessage (e.g. failing to +// dedupe the mirrored copy) left `go test ./pkg/app` green (#3584 review, +// mutation-testing proof). Every entry point must deliver exactly one +// dialog for both a mirrored/local-shaped runtime (sink AND RunStream both +// fire; the RunStream copy must be skipped) and an unmirrored/remote-shaped +// runtime (RunStream is the only delivery; it must pass through). +func TestElicitationDeliveryAcrossEntryPoints(t *testing.T) { + t.Parallel() + + for _, ep := range elicitationEntryPoints { + t.Run(ep.name, func(t *testing.T) { + t.Parallel() + + t.Run("mirrored runtime delivers exactly once", func(t *testing.T) { + t.Parallel() + + rt := &composedElicitationMockRuntime{} + events := make(chan tea.Msg, 16) + app := &App{runtime: rt, session: session.New(), events: events} + app.Start(t.Context()) + require.NotNil(t, rt.handler, "Start must register the OnElicitationRequest handler") - assert.Equal(t, 1, elicitations, "a single foreground elicitation must open exactly one dialog") + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + ep.invoke(app, ctx, cancel) + + assert.Equal(t, 1, countElicitationDeliveries(t, events), + "a mirrored/local-shaped runtime's foreground elicitation must open exactly one dialog") + }) + + t.Run("unmirrored runtime still delivers once", func(t *testing.T) { + t.Parallel() + + rt := &remoteLikeMockRuntime{} + events := make(chan tea.Msg, 16) + app := &App{runtime: rt, session: session.New(), events: events} + app.Start(t.Context()) + + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + ep.invoke(app, ctx, cancel) + + assert.Equal(t, 1, countElicitationDeliveries(t, events), + "an unmirrored/remote-shaped runtime's elicitation must still reach the app via RunStream") + }) + }) + } } From f445d3e0184726b80a3511facc0cb044700b7e75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arnaud=20He=CC=81ritier?= Date: Wed, 15 Jul 2026 22:11:48 +0200 Subject: [PATCH 5/5] fix(tui,app): make elicitation-ID params variadic for backward compatibility (#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 (bebfc22b1). 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. --- pkg/app/app.go | 10 ++++++---- pkg/tui/dialog/elicitation.go | 22 +++++++++++++++++++--- pkg/tui/dialog/oauth_authorization.go | 9 ++++++--- pkg/tui/dialog/url_elicitation.go | 9 ++++++--- pkg/tui/page/chat/runtime_events.go | 2 +- pkg/tui/tui.go | 2 +- 6 files changed, 39 insertions(+), 15 deletions(-) diff --git a/pkg/app/app.go b/pkg/app/app.go index d488d76a17..22c2de5597 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -1023,10 +1023,12 @@ func (a *App) TogglePause() (paused, supported bool) { } // ResumeElicitation resumes an elicitation request with the given action and -// content. elicitationID is additive: pass "" to fall back to resolving the -// sole pending request (see runtime.Runtime.ResumeElicitation). -func (a *App) ResumeElicitation(ctx context.Context, action tools.ElicitationAction, content map[string]any, elicitationID string) error { - return a.runtime.ResumeElicitation(ctx, action, content, elicitationID) +// content. elicitationID is variadic, mirroring runtime.Runtime.ResumeElicitation, +// purely so pre-#3584 3-arg callers keep compiling unchanged; at most the +// first value is meaningful, and "" (or omitting it) falls back to resolving +// the sole pending request. +func (a *App) ResumeElicitation(ctx context.Context, action tools.ElicitationAction, content map[string]any, elicitationID ...string) error { + return a.runtime.ResumeElicitation(ctx, action, content, elicitationID...) } func (a *App) NewSession() { diff --git a/pkg/tui/dialog/elicitation.go b/pkg/tui/dialog/elicitation.go index 70d9c9f41a..ee95aa23ee 100644 --- a/pkg/tui/dialog/elicitation.go +++ b/pkg/tui/dialog/elicitation.go @@ -119,9 +119,25 @@ func (d *ElicitationDialog) hasFreeFormInput() bool { return len(d.fields) == 0 } -// NewElicitationDialog creates a new elicitation dialog. -func NewElicitationDialog(message string, schema any, meta map[string]any, elicitationID string) Dialog { +// firstElicitationID extracts the (at most one meaningful) elicitation ID +// from a variadic parameter. Shared by the elicitation dialog constructors +// in this package, whose elicitationID parameter is variadic rather than a +// required positional string purely so pre-#3584 3-arg call sites keep +// compiling unchanged (mirrors pkg/runtime.firstElicitationID). +func firstElicitationID(elicitationID []string) string { + if len(elicitationID) == 0 { + return "" + } + return elicitationID[0] +} + +// NewElicitationDialog creates a new elicitation dialog. elicitationID is +// variadic purely so pre-existing 3-arg callers keep compiling unchanged +// (see firstElicitationID for the same #3584 precedent); at most the first +// value is meaningful. +func NewElicitationDialog(message string, schema any, meta map[string]any, elicitationID ...string) Dialog { fields := parseElicitationSchema(schema) + id := firstElicitationID(elicitationID) // Determine dialog title from meta, defaulting to "Question" title := "Question" @@ -134,7 +150,7 @@ func NewElicitationDialog(message string, schema any, meta map[string]any, elici d := &ElicitationDialog{ title: title, message: message, - elicitationID: elicitationID, + elicitationID: id, fields: fields, inputs: make([]textinput.Model, len(fields)), boolValues: make(map[int]bool), diff --git a/pkg/tui/dialog/oauth_authorization.go b/pkg/tui/dialog/oauth_authorization.go index f3707d2847..339f290e1d 100644 --- a/pkg/tui/dialog/oauth_authorization.go +++ b/pkg/tui/dialog/oauth_authorization.go @@ -24,12 +24,15 @@ type oauthAuthorizationDialog struct { keyMap ConfirmKeyMap } -// NewOAuthAuthorizationDialog creates a new OAuth authorization confirmation dialog -func NewOAuthAuthorizationDialog(ctx context.Context, serverURL, elicitationID string, appInstance *app.App) Dialog { +// NewOAuthAuthorizationDialog creates a new OAuth authorization confirmation dialog. +// elicitationID is variadic for the same backward-compatibility reason as +// NewElicitationDialog (see firstElicitationID); at most the first value is +// meaningful. +func NewOAuthAuthorizationDialog(ctx context.Context, serverURL string, appInstance *app.App, elicitationID ...string) Dialog { return &oauthAuthorizationDialog{ ctx: func() context.Context { return context.WithoutCancel(ctx) }, serverURL: serverURL, - elicitationID: elicitationID, + elicitationID: firstElicitationID(elicitationID), app: appInstance, keyMap: DefaultConfirmKeyMap(), } diff --git a/pkg/tui/dialog/url_elicitation.go b/pkg/tui/dialog/url_elicitation.go index dec734bb0a..5c677152e6 100644 --- a/pkg/tui/dialog/url_elicitation.go +++ b/pkg/tui/dialog/url_elicitation.go @@ -28,13 +28,16 @@ type URLElicitationDialog struct { openBrowser key.Binding } -// NewURLElicitationDialog creates a new URL elicitation dialog. -func NewURLElicitationDialog(ctx context.Context, message, url, elicitationID string) Dialog { +// NewURLElicitationDialog creates a new URL elicitation dialog. elicitationID +// is variadic for the same backward-compatibility reason as +// NewElicitationDialog (see firstElicitationID); at most the first value is +// meaningful. +func NewURLElicitationDialog(ctx context.Context, message, url string, elicitationID ...string) Dialog { return &URLElicitationDialog{ ctx: func() context.Context { return context.WithoutCancel(ctx) }, message: message, url: url, - elicitationID: elicitationID, + elicitationID: firstElicitationID(elicitationID), keyMap: DefaultConfirmKeyMap(), escape: key.NewBinding(key.WithKeys("esc")), openBrowser: key.NewBinding( diff --git a/pkg/tui/page/chat/runtime_events.go b/pkg/tui/page/chat/runtime_events.go index 60835fd887..4502807e52 100644 --- a/pkg/tui/page/chat/runtime_events.go +++ b/pkg/tui/page/chat/runtime_events.go @@ -449,7 +449,7 @@ func (p *chatPage) handleElicitationRequest(msg *runtime.ElicitationRequestEvent serverURL = url } dialogCmd := core.CmdHandler(dialog.OpenDialogMsg{ - Model: dialog.NewOAuthAuthorizationDialog(p.ctx(), serverURL, msg.ElicitationID, p.app), + Model: dialog.NewOAuthAuthorizationDialog(p.ctx(), serverURL, p.app, msg.ElicitationID), OriginatingEvent: msg, }) return tea.Batch(spinnerCmd, dialogCmd) diff --git a/pkg/tui/tui.go b/pkg/tui/tui.go index c249306cfd..ae07e3f29e 100644 --- a/pkg/tui/tui.go +++ b/pkg/tui/tui.go @@ -1902,7 +1902,7 @@ func (m *appModel) replayElicitationEvent(ev *runtime.ElicitationRequestEvent) t serverURL = url } return core.CmdHandler(dialog.OpenDialogMsg{ - Model: dialog.NewOAuthAuthorizationDialog(m.ctx(), serverURL, ev.ElicitationID, m.application), + Model: dialog.NewOAuthAuthorizationDialog(m.ctx(), serverURL, m.application, ev.ElicitationID), OriginatingEvent: ev, }) }