diff --git a/README.md b/README.md index 2375b077b5..3d32995451 100644 --- a/README.md +++ b/README.md @@ -350,6 +350,7 @@ Auto-approve mode skips interactive permission dialogs for `ask`-resolved permis - **Scope**: per-session only — new sessions start without auto-approve - **Subagents**: child task sessions inherit auto-approve from the parent - **Non-interactive mode**: already auto-approves all permissions, flag is ignored +- **Questions still ask**: auto-approve covers tool permissions, not decisions — the `question` tool keeps prompting you (TUI dialog, chat bridge, API). It answers itself with the first (recommended) option only where nobody could answer: a headless `opencode -p` run, a flow step, or a cron job firing on an unwatched session ### Shell diff --git a/cmd/drain_notifier.go b/cmd/drain_notifier.go new file mode 100644 index 0000000000..9657133296 --- /dev/null +++ b/cmd/drain_notifier.go @@ -0,0 +1,79 @@ +package cmd + +import ( + "sync" + + tea "charm.land/bubbletea/v2" + appPkg "github.com/opencode-ai/opencode/internal/app" + "github.com/opencode-ai/opencode/internal/logging" +) + +// drainEventBuffer is the depth of the forwarder queue. Drain events are +// low-rate (one per enqueue / delivery / discard), so this is generous enough +// that the out-of-band spill path below is effectively unreachable. +const drainEventBuffer = 128 + +// newDrainForwarder adapts App.SetDrainNotifier to tea.Program.Send. +// +// It exists because Program.Send MUST NOT be called from the Bubble Tea update +// goroutine: p.msgs is unbuffered and is only read by the event loop, which is +// itself blocked inside Model.Update while the update runs. A bare +// `program.Send(e)` notifier therefore deadlocks the entire TUI permanently the +// first time a user submits a message while the agent is busy — the editor's +// enqueue path (chat/editor.go send) and the ctrl+x discard path both call into +// App.EnqueueMessage / App.DiscardQueue synchronously from Update, and those +// notify the registered callback before returning. Bubble Tea itself always +// wraps internal self-sends in `go p.Send(...)` for the same reason. +// +// The returned notify never blocks its caller and preserves event order: events +// are handed to a single forwarder goroutine over a buffered channel, mirroring +// how service subscriptions reach the TUI (see setupSubscriptions). stop halts +// the forwarder and waits for it to exit. +func newDrainForwarder(send func(tea.Msg)) (notify func(appPkg.DrainEvent), stop func()) { + events := make(chan appPkg.DrainEvent, drainEventBuffer) + stopped := make(chan struct{}) + exited := make(chan struct{}) + var stopOnce sync.Once + + go func() { + defer close(exited) + defer logging.RecoverPanic("TUI-drain-forwarder", nil) + + for { + select { + case <-stopped: + return + case e := <-events: + send(e) + } + } + }() + + notify = func(e appPkg.DrainEvent) { + select { + case events <- e: + case <-stopped: + default: + // Buffer full: hand off to a goroutine rather than blocking the + // caller, which may be the Bubble Tea update goroutine. Delivery + // is preserved (error events are the only signal a halted drain + // worker ever emits) at the cost of ordering under backpressure — + // harmless, since the queue banner re-reads QueueLen in View. + logging.Warn("drain event forwarder buffer full, delivering out of band", + "session", e.SessionID) + go func() { + select { + case events <- e: + case <-stopped: + } + }() + } + } + + stop = func() { + stopOnce.Do(func() { close(stopped) }) + <-exited + } + + return notify, stop +} diff --git a/cmd/drain_notifier_test.go b/cmd/drain_notifier_test.go new file mode 100644 index 0000000000..16467c3a09 --- /dev/null +++ b/cmd/drain_notifier_test.go @@ -0,0 +1,157 @@ +package cmd + +import ( + "io" + "strings" + "sync" + "testing" + "time" + + tea "charm.land/bubbletea/v2" + appPkg "github.com/opencode-ai/opencode/internal/app" +) + +// Regression test for the queue-message freeze: the notifier registered on the +// App is invoked synchronously from the Bubble Tea update goroutine (editor +// enqueue, ctrl+x discard). If it forwards straight to tea.Program.Send it +// blocks forever, because Send writes to an unbuffered channel that only the +// event loop reads — and the event loop is inside Update, waiting for the +// notifier to return. The forwarder must never block its caller. +func TestDrainForwarder_NotifyNeverBlocksCaller(t *testing.T) { + release := make(chan struct{}) + var sent []appPkg.DrainEvent + var mu sync.Mutex + + blockingSend := func(msg tea.Msg) { + <-release // stands in for the wedged event loop + mu.Lock() + defer mu.Unlock() + sent = append(sent, msg.(appPkg.DrainEvent)) + } + + notify, stop := newDrainForwarder(blockingSend) + defer func() { + close(release) + stop() + }() + + done := make(chan struct{}) + go func() { + defer close(done) + for i := 0; i < drainEventBuffer+8; i++ { + notify(appPkg.DrainEvent{SessionID: "s1", QueueLen: i}) + } + }() + + select { + case <-done: + case <-time.After(3 * time.Second): + t.Fatal("notify blocked while the consumer was stalled — the TUI would freeze") + } +} + +// Once the event loop is free again, buffered events are delivered in order. +func TestDrainForwarder_DeliversInOrder(t *testing.T) { + const n = 16 + + got := make(chan int, n) + notify, stop := newDrainForwarder(func(msg tea.Msg) { + got <- msg.(appPkg.DrainEvent).QueueLen + }) + defer stop() + + for i := 0; i < n; i++ { + notify(appPkg.DrainEvent{SessionID: "s1", QueueLen: i}) + } + + for i := 0; i < n; i++ { + select { + case v := <-got: + if v != i { + t.Fatalf("event %d delivered out of order: got QueueLen=%d", i, v) + } + case <-time.After(3 * time.Second): + t.Fatalf("event %d never delivered", i) + } + } +} + +// stop is idempotent and safe to call while notify is still being used, and +// post-stop notifies are dropped rather than panicking on a closed channel. +func TestDrainForwarder_StopIsSafe(t *testing.T) { + notify, stop := newDrainForwarder(func(tea.Msg) {}) + + notify(appPkg.DrainEvent{SessionID: "s1"}) + stop() + stop() + + done := make(chan struct{}) + go func() { + defer close(done) + notify(appPkg.DrainEvent{SessionID: "s1"}) + }() + + select { + case <-done: + case <-time.After(3 * time.Second): + t.Fatal("notify blocked after stop") + } +} + +// End-to-end version of the freeze against a real tea.Program: notifying from +// inside Model.Update (what EnqueueMessage / DiscardQueue do) must return. With +// a raw `program.Send` notifier this test hangs until the timeout, which is the +// exact user-visible bug — an unrecoverable TUI freeze. +func TestDrainForwarder_NotifyFromUpdateDoesNotDeadlock(t *testing.T) { + m := &forwarderProbeModel{ + entered: make(chan struct{}, 1), + returned: make(chan struct{}, 1), + } + program := tea.NewProgram(m, + tea.WithInput(strings.NewReader("")), + tea.WithOutput(io.Discard), + ) + notify, stop := newDrainForwarder(program.Send) + defer stop() + m.notify = notify + + go func() { _, _ = program.Run() }() + // Quit is itself a Send, so it deadlocks too when the bug is present — + // fire it off-goroutine so a regression fails on the timeouts below + // instead of hanging the whole test binary. + defer func() { go program.Quit() }() + + select { + case <-m.entered: + case <-time.After(5 * time.Second): + t.Fatal("Update never ran") + } + select { + case <-m.returned: + case <-time.After(3 * time.Second): + t.Fatal("notify from inside Update never returned — the TUI is deadlocked") + } +} + +type forwarderProbeMsg struct{} + +type forwarderProbeModel struct { + notify func(appPkg.DrainEvent) + entered chan struct{} + returned chan struct{} +} + +func (m *forwarderProbeModel) Init() tea.Cmd { + return func() tea.Msg { return forwarderProbeMsg{} } +} + +func (m *forwarderProbeModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { + if _, ok := msg.(forwarderProbeMsg); ok { + m.entered <- struct{}{} + m.notify(appPkg.DrainEvent{SessionID: "s1", QueueLen: 1}) + m.returned <- struct{}{} + } + return m, nil +} + +func (m *forwarderProbeModel) View() tea.View { return tea.NewView("") } diff --git a/cmd/flow.go b/cmd/flow.go index 92562a3579..d48ff5aa8b 100644 --- a/cmd/flow.go +++ b/cmd/flow.go @@ -73,6 +73,10 @@ func runNonInteractive(ctx context.Context, a *app.App, prompt string, outputFor } a.Permissions.AutoApproveSession(sess.ID) + // Nobody is watching a headless `-p` run: there is no TUI dialog and no + // chat binding, so the question tool must answer itself instead of + // blocking forever on a prompt no human will ever see. + a.Permissions.MarkUnattendedSession(sess.ID) // Headless prompt invocation is non-interactive: hold the turn open // until background tasks (bash run_in_background, task async, monitor) diff --git a/cmd/root.go b/cmd/root.go index 7c68d619a6..a87ca3931f 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -8,7 +8,7 @@ import ( "time" tea "charm.land/bubbletea/v2" - "github.com/opencode-ai/opencode/internal/app" + appPkg "github.com/opencode-ai/opencode/internal/app" "github.com/opencode-ai/opencode/internal/config" "github.com/opencode-ai/opencode/internal/db" "github.com/opencode-ai/opencode/internal/flow" @@ -174,7 +174,7 @@ to assist developers in writing, debugging, and understanding code directly from // Create main context for the application ctx, cancel := context.WithCancel(context.Background()) defer cancel() - app, err := app.New(ctx, conn, cliSchema, projectID) + app, err := appPkg.New(ctx, conn, cliSchema, projectID) if err != nil { if spinner != nil { spinner.Stop() @@ -269,6 +269,14 @@ to assist developers in writing, debugging, and understanding code directly from tui.New(app), ) + // Wire the drain notifier: drain workers and the editor's enqueue path + // push DrainEvents (queue count updates and attributed errors) into the + // TUI event loop. The forwarder goroutine is mandatory — notifying from + // the Bubble Tea update goroutine with a bare program.Send deadlocks the + // TUI (see newDrainForwarder). + drainNotify, stopDrainForwarder := newDrainForwarder(program.Send) + app.SetDrainNotifier(drainNotify) + // Setup the subscriptions, this will send services events to the TUI ch, permCh, cancelSubs := setupSubscriptions(app, ctx) @@ -340,6 +348,10 @@ to assist developers in writing, debugging, and understanding code directly from }() cleanupWg.Wait() + // Drain workers are stopped by app.Shutdown above, so no further + // events can be emitted; release the forwarder goroutine. + stopDrainForwarder() + logging.Info("All goroutines cleaned up") } @@ -446,7 +458,7 @@ func setupBlockingSubscriber[T any]( }() } -func setupSubscriptions(app *app.App, parentCtx context.Context) (chan tea.Msg, chan tea.Msg, func()) { +func setupSubscriptions(app *appPkg.App, parentCtx context.Context) (chan tea.Msg, chan tea.Msg, func()) { ch := make(chan tea.Msg, 100) permCh := make(chan tea.Msg, 10) diff --git a/cmd/schema/main.go b/cmd/schema/main.go index 354010c5b2..eeb62348dc 100644 --- a/cmd/schema/main.go +++ b/cmd/schema/main.go @@ -998,5 +998,59 @@ func generateSchema() map[string]any { }, } + // Add router (chat-bridge) configuration. When updating this schema, also + // update internal/bridge/config.go (bridge.Config) and docs/bridge.md. + schema["properties"].(map[string]any)["router"] = map[string]any{ + "type": "object", + "description": "Chat-bridge configuration. The bridge connects opencode to Telegram, Slack, and Mattermost. Set at least one channel identity to enable.", + "properties": map[string]any{ + "questionMode": map[string]any{ + "type": "string", + "description": "How agent questions are surfaced: 'interactive' renders platform-native UI (buttons/blocks); 'auto-reject' returns the default without prompting; 'disabled' suppresses the question flow.", + "enum": []string{"interactive", "auto-reject", "disabled"}, + }, + "permissionMode": map[string]any{ + "type": "string", + "description": "How permission requests are resolved on bridge-owned sessions: 'allow' auto-approves, 'deny' auto-denies, 'ask' or empty defers to the opencode UI (hangs headless). Unrecognised values fail-safe to deny.", + "enum": []string{"allow", "deny", "ask"}, + }, + "toolUpdatesEnabled": map[string]any{ + "type": "boolean", + "description": "Stream tool-call lifecycle events (pending/running/completed) to the chat surface. Failures always surface regardless of this flag.", + "default": false, + }, + "toolUpdateVerbosity": map[string]any{ + "type": "string", + "description": "Detail level when toolUpdatesEnabled is true: 'compact' (default) emits one line per call with glyph, name, and elapsed time; 'full' adds argument and result detail.", + "enum": []string{"compact", "full"}, + "default": "compact", + }, + "questionNudgeIntervalSeconds": map[string]any{ + "type": "integer", + "description": "Idle gap (seconds) after which the bridge re-posts a 'still waiting' nudge to a session with an outstanding question. 0 = built-in default (300 s); <0 = disable nudging.", + }, + "questionNudgeMax": map[string]any{ + "type": "integer", + "description": "Maximum nudges per pending question. 0 = built-in default (3); <0 = unlimited.", + }, + "queueAcknowledgementsEnabled": map[string]any{ + "type": "boolean", + "description": "When true, the bridge sends an in-place-editable '⏳ queued' acknowledgement to a sender whose message is enqueued behind an in-flight agent run. The ack is updated as the queue drains and resolved to '▶ Processing…' when the run starts. Disabled by default; enable for reviewers who need visibility into queue depth.", + "default": false, + }, + "channels": map[string]any{ + "type": "object", + "description": "Per-platform channel configuration. See docs/bridge.md for field details.", + "properties": map[string]any{ + "telegram": map[string]any{"type": "object", "description": "Telegram channel configuration."}, + "slack": map[string]any{"type": "object", "description": "Slack channel configuration."}, + "mattermost": map[string]any{"type": "object", "description": "Mattermost channel configuration."}, + "external": map[string]any{"type": "object", "description": "External relay channel configuration."}, + }, + }, + }, + "additionalProperties": false, + } + return schema } diff --git a/docs/bridge.md b/docs/bridge.md index f215bdb3bb..a55476743a 100644 --- a/docs/bridge.md +++ b/docs/bridge.md @@ -83,6 +83,7 @@ Health snapshot: `curl http://127.0.0.1:3456/router/health` (per-adapter `status | `permissionMode` | `"allow"` \| `"deny"` \| `"ask"` \| empty | How the bridge resolves agent permission requests on bridge-bound sessions. `allow`/`deny` auto-resolve; `ask`/empty defer to opencode's default UI (will hang headless). Unrecognised values fail-safe to deny with a one-shot WARN log. | | `toolUpdatesEnabled` | `bool` | Stream tool-call lifecycle to chat. Detail level is set by `toolUpdateVerbosity`. Failures surface regardless of this flag. | | `toolUpdateVerbosity` | `"compact"` (default) \| `"full"` | `compact` emits **one line per tool call**: `🔧 #` while running, updated in place to `✓ # · ` on completion — arguments and result bodies stay out of chat (they're in the session store and Langfuse). Failures always append a truncated reason: `✗ # · · `. `full` restores the argument summary on the call line and a truncated result body on completion. Unrecognised values fall back to `compact` with a one-shot WARN. Flip it live with `/verbosity`. | +| `queueAcknowledgementsEnabled` | `bool` | When `true`, sends an in-place-editable `⏳ queued` acknowledgement to a sender whose message is enqueued behind an in-flight agent run. The ack is edited as the queue drains and resolved to `▶ Processing your message now…` the moment the run starts. Requires 2 seconds of queuing before sending, to avoid a pointless flash for sub-second waits. Default: `false`. All three production adapters (Telegram, Slack, Mattermost) support in-place edit; the external adapter silently skips acks. | | `channels.{telegram,slack,mattermost,external}` | object | Per-platform configuration; see below. | ## Per-channel configuration diff --git a/docs/crons.md b/docs/crons.md index bb8b92c8a2..4ff035d1f0 100644 --- a/docs/crons.md +++ b/docs/crons.md @@ -174,6 +174,8 @@ A cron job lives in a session. If that session isn't the active one in the TUI a Auto-approved jobs and jobs with explicit `cron: allow` rules run regardless of which session is active. +A job that fires on a session nothing is watching (not the TUI's selected session, not bridge-bound) runs **unattended**: if its agent calls the `question` tool, the tool answers itself with the first (recommended) option instead of blocking on a prompt nobody would see. The verdict is re-evaluated on every fire, so focusing the session in the TUI — or binding chat to it — brings the real prompt back. + ### Session became busy after task ran The scheduler tries to commit the synthetic `task_call`/`task_result` pair into the parent session atomically — it briefly holds the session-busy slot to prevent the parent agent from inserting a message in between. If a user message arrives during the narrow window between "we ran the task" and "we got the lock", the synthetic write is skipped and the result is preserved only on the cron row (visible via the crons page). The job still advances `next_run_at` correctly; it does not re-fire. diff --git a/internal/app/app.go b/internal/app/app.go index 57b424f3cb..b8e0048bbf 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -8,6 +8,7 @@ import ( "os/exec" "strconv" "strings" + "sync" "sync/atomic" agentregistry "github.com/opencode-ai/opencode/internal/agent" @@ -66,6 +67,14 @@ type App struct { activeSessionID atomic.Value // stores string cliOutputSchema map[string]any + + // Per-session in-memory message queue and drain-worker lifecycle. + ctx context.Context + queues map[string][]QueuedMessage + queueMu sync.Mutex + queueCancels map[string]context.CancelFunc + queueWg sync.WaitGroup + drainNotify func(DrainEvent) } // SetActiveSessionID is called by the TUI whenever the selected session changes. @@ -203,6 +212,9 @@ func New(ctx context.Context, conn *sql.DB, cliSchema map[string]any, projectID Crons: cronSvc, Todos: todoStore, Questions: questionSvc, + ctx: ctx, + queues: make(map[string][]QueuedMessage), + queueCancels: make(map[string]context.CancelFunc), } // Install the global background-task registry. EnqueueTaskCompletion @@ -323,6 +335,7 @@ func (app *App) initTheme() { // Shutdown performs a clean shutdown of the application func (app *App) Shutdown() { + app.ShutdownQueues() if app.CronScheduler != nil { app.CronScheduler.Stop() } diff --git a/internal/app/drain_test.go b/internal/app/drain_test.go new file mode 100644 index 0000000000..f26249d407 --- /dev/null +++ b/internal/app/drain_test.go @@ -0,0 +1,473 @@ +package app + +import ( + "context" + "errors" + "fmt" + "sync" + "sync/atomic" + "testing" + "time" + + agentpkg "github.com/opencode-ai/opencode/internal/llm/agent" + "github.com/opencode-ai/opencode/internal/llm/models" + "github.com/opencode-ai/opencode/internal/llm/tools" + "github.com/opencode-ai/opencode/internal/message" + "github.com/opencode-ai/opencode/internal/pubsub" + + "github.com/opencode-ai/opencode/internal/config" +) + +// ---- fakeAgent stub --------------------------------------------------------- + +// fakeAgent is a minimal stub of agent.Service. Only the methods called by the +// drain loop need real implementations; the rest panic if called unexpectedly. +type fakeAgent struct { + mu sync.Mutex + results []runResult +} + +type runResult struct { + err error +} + +func (f *fakeAgent) setResults(rs ...runResult) { + f.mu.Lock() + defer f.mu.Unlock() + f.results = rs +} + +func (f *fakeAgent) IsSessionBusy(_ string) bool { return false } + +func (f *fakeAgent) Run(_ context.Context, _ string, _ string, _ int, _ ...message.Attachment) (<-chan agentpkg.AgentEvent, error) { + f.mu.Lock() + defer f.mu.Unlock() + if len(f.results) == 0 { + ch := make(chan agentpkg.AgentEvent) + close(ch) + return ch, nil + } + r := f.results[0] + f.results = f.results[1:] + if r.err != nil { + return nil, r.err + } + ch := make(chan agentpkg.AgentEvent) + close(ch) + return ch, nil +} + +// Satisfy the rest of the interface: +func (f *fakeAgent) Subscribe(_ context.Context) <-chan pubsub.Event[agentpkg.AgentEvent] { + ch := make(chan pubsub.Event[agentpkg.AgentEvent]) + close(ch) + return ch +} +func (f *fakeAgent) AgentID() config.AgentName { return "" } +func (f *fakeAgent) Model() models.Model { return models.Model{} } +func (f *fakeAgent) Tools() []tools.BaseTool { return nil } +func (f *fakeAgent) ResolvedTools() ([]tools.BaseTool, bool) { return nil, false } +func (f *fakeAgent) RunWith(_ context.Context, _ string, _ string, _ int, _ agentpkg.RunOptions, _ ...message.Attachment) (<-chan agentpkg.AgentEvent, error) { + return nil, nil +} +func (f *fakeAgent) Cancel(_ string) {} +func (f *fakeAgent) IsBusy() bool { return false } +func (f *fakeAgent) TryLockSession(_ string) bool { return true } +func (f *fakeAgent) UnlockSession(_ string) {} +func (f *fakeAgent) Update(_ config.AgentName, _ models.ModelID) (models.Model, error) { + return models.Model{}, nil +} +func (f *fakeAgent) Summarize(_ context.Context, _ string) error { return nil } +func (f *fakeAgent) SummarizeSync(_ context.Context, _ string) error { return nil } +func (f *fakeAgent) GenerateRecap(_ context.Context, _ string) (string, error) { return "", nil } + +// ---- recordAgent ------------------------------------------------------------ + +// recordAgent extends fakeAgent to record Run call texts in order. +type recordAgent struct { + fakeAgent + callMu sync.Mutex + calls []string +} + +func (r *recordAgent) Run(ctx context.Context, sid string, content string, max int, a ...message.Attachment) (<-chan agentpkg.AgentEvent, error) { + r.callMu.Lock() + r.calls = append(r.calls, content) + r.callMu.Unlock() + return r.fakeAgent.Run(ctx, sid, content, max, a...) +} + +// ---- pauseAgent ------------------------------------------------------------- + +// pauseAgent blocks each Run until its release channel is closed. +type pauseAgent struct { + fakeAgent + callMu sync.Mutex + calls []string + release chan struct{} +} + +func (p *pauseAgent) Run(ctx context.Context, _ string, content string, _ int, _ ...message.Attachment) (<-chan agentpkg.AgentEvent, error) { + p.callMu.Lock() + p.calls = append(p.calls, content) + p.callMu.Unlock() + + select { + case <-p.release: + case <-ctx.Done(): + return nil, ctx.Err() + } + ch := make(chan agentpkg.AgentEvent) + close(ch) + return ch, nil +} + +// ---- countRunAgent ---------------------------------------------------------- + +// countRunAgent counts Run calls and consumes from a preset result list. +type countRunAgent struct { + fakeAgent + count atomic.Int32 + resMu sync.Mutex + preset []runResult +} + +func (c *countRunAgent) Run(ctx context.Context, sid string, content string, max int, a ...message.Attachment) (<-chan agentpkg.AgentEvent, error) { + c.count.Add(1) + c.resMu.Lock() + if len(c.preset) > 0 { + r := c.preset[0] + c.preset = c.preset[1:] + c.resMu.Unlock() + if r.err != nil { + return nil, r.err + } + ch := make(chan agentpkg.AgentEvent) + close(ch) + return ch, nil + } + c.resMu.Unlock() + return c.fakeAgent.Run(ctx, sid, content, max, a...) +} + +// ---- drainCapture ----------------------------------------------------------- + +type drainCapture struct { + mu sync.Mutex + events []DrainEvent +} + +func newDrainApp(ctx context.Context, ag agentpkg.Service) (*App, *drainCapture) { + a := newTestApp(ctx) + a.activeAgent = ag + cap := &drainCapture{} + a.SetDrainNotifier(func(e DrainEvent) { + cap.mu.Lock() + defer cap.mu.Unlock() + cap.events = append(cap.events, e) + }) + return a, cap +} + +func (c *drainCapture) errors() []error { + c.mu.Lock() + defer c.mu.Unlock() + var errs []error + for _, e := range c.events { + if e.Err != nil { + errs = append(errs, e.Err) + } + } + return errs +} + +// ---- helpers ---------------------------------------------------------------- + +// waitFor polls cond until it returns true or times out. +func waitFor(t *testing.T, desc string, cond func() bool) { + t.Helper() + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + if cond() { + return + } + time.Sleep(10 * time.Millisecond) + } + t.Fatalf("timeout waiting for: %s", desc) +} + +// ---- tests ------------------------------------------------------------------ + +// TestDrain_FIFO_Simple asserts that three queued messages are delivered in +// enqueue order (FIFO). +func TestDrain_FIFO_Simple(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + ra := &recordAgent{} + a, _ := newDrainApp(ctx, ra) + + sid := "fifo-simple" + a.queueMu.Lock() + a.queues[sid] = []QueuedMessage{{Text: "first"}, {Text: "second"}, {Text: "third"}} + a.startDrainWorker(sid) + a.queueMu.Unlock() + + waitFor(t, "queue empties", func() bool { return a.QueueLen(sid) == 0 }) + a.queueWg.Wait() + + ra.callMu.Lock() + got := ra.calls + ra.callMu.Unlock() + + want := []string{"first", "second", "third"} + if len(got) != len(want) { + t.Fatalf("Run calls = %v, want %v", got, want) + } + for i, w := range want { + if got[i] != w { + t.Errorf("call[%d] = %q, want %q (FIFO violation)", i, got[i], w) + } + } +} + +// TestDrain_ErrSessionBusy_Retry_Count asserts ErrSessionBusy triggers a retry +// without surfacing an error to the user. +func TestDrain_ErrSessionBusy_Retry_Count(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + ca := &countRunAgent{} + ca.preset = []runResult{ + {err: agentpkg.ErrSessionBusy}, + {err: nil}, + } + + a, cap := newDrainApp(ctx, ca) + + sid := "busy-retry-count" + a.queueMu.Lock() + a.queues[sid] = []QueuedMessage{{Text: "retried"}} + a.startDrainWorker(sid) + a.queueMu.Unlock() + + waitFor(t, "queue empties after retry", func() bool { return a.QueueLen(sid) == 0 }) + a.queueWg.Wait() + + if got := ca.count.Load(); got < 2 { + t.Errorf("expected ≥2 Run calls (busy+retry), got %d", got) + } + if errs := cap.errors(); len(errs) != 0 { + t.Errorf("unexpected drain errors after ErrSessionBusy: %v", errs) + } +} + +// TestDrain_NonBusyError_HaltsWorker asserts a non-ErrSessionBusy error from +// Run surfaces an attributed error, halts the drain worker, preserves the +// remaining queue (including the failed message re-prepended at head), and +// allows a fresh EnqueueMessage to start a new worker. +func TestDrain_NonBusyError_HaltsWorker(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + providerErr := errors.New("provider API down") + ca := &countRunAgent{} + ca.preset = []runResult{{err: providerErr}} + + a, cap := newDrainApp(ctx, ca) + + sid := "halt-test" + a.queueMu.Lock() + a.queues[sid] = []QueuedMessage{{Text: "fails"}, {Text: "survivor"}} + a.startDrainWorker(sid) + a.queueMu.Unlock() + + // Worker halts after one error. + waitFor(t, "worker halts after error", func() bool { + a.queueMu.Lock() + _, running := a.queueCancels[sid] + a.queueMu.Unlock() + return !running + }) + a.queueWg.Wait() + + // Error must be attributed and wrap providerErr. + errs := cap.errors() + if len(errs) == 0 { + t.Fatal("expected an attributed error notification, got none") + } + if !errors.Is(errs[0], providerErr) { + t.Errorf("error should wrap providerErr, got: %v", errs[0]) + } + const prefix = "queued message could not be delivered" + if errStr := errs[0].Error(); len(errStr) < len(prefix) || errStr[:len(prefix)] != prefix { + t.Errorf("error attribution missing: %q", errStr) + } + + // Both messages must still be in the queue. + if remaining := a.QueueLen(sid); remaining < 2 { + t.Errorf("expected ≥2 messages remaining, got %d", remaining) + } + + // Fresh enqueue starts a new worker. + a.activeAgent = &fakeAgent{} + a.EnqueueMessage(sid, QueuedMessage{Text: "new-trigger"}) + a.queueMu.Lock() + _, running := a.queueCancels[sid] + a.queueMu.Unlock() + if !running { + t.Error("expected a fresh drain worker after re-enqueue post-halt") + } + a.ShutdownQueues() +} + +// TestDrain_WorkerTerminatesAfterEmptyQueue asserts the worker goroutine exits +// after draining without leaking. +func TestDrain_WorkerTerminatesAfterEmptyQueue(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + ra := &recordAgent{} + a, _ := newDrainApp(ctx, ra) + + sid := "terminates" + a.queueMu.Lock() + a.queues[sid] = []QueuedMessage{{Text: "only"}} + a.startDrainWorker(sid) + a.queueMu.Unlock() + + done := make(chan struct{}) + go func() { + a.queueWg.Wait() + close(done) + }() + + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("worker did not exit after draining (goroutine leak)") + } + + a.queueMu.Lock() + _, running := a.queueCancels[sid] + a.queueMu.Unlock() + if running { + t.Error("queueCancels entry should be removed after worker exits") + } +} + +// TestDrain_ContextCancellation_ExitsWorker asserts the worker exits promptly +// when its context is cancelled, even with a non-empty queue. +func TestDrain_ContextCancellation_ExitsWorker(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + + pa := &pauseAgent{release: make(chan struct{})} + a, _ := newDrainApp(ctx, pa) + + sid := "cancel-test" + a.queueMu.Lock() + a.queues[sid] = []QueuedMessage{{Text: "blocks"}, {Text: "never-runs"}} + a.startDrainWorker(sid) + a.queueMu.Unlock() + + // Give the worker a moment to start the first Run call. + time.Sleep(20 * time.Millisecond) + + cancel() // context cancellation signals shutdown + + done := make(chan struct{}) + go func() { + a.queueWg.Wait() + close(done) + }() + + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("worker did not exit on context cancellation") + } +} + +// TestDrain_FIFO_QueueNonEmptySlotMomentarilyIdle verifies FIFO is preserved +// when a new message arrives while the queue has entries but the slot is +// momentarily free (between drain deliveries). The enqueue path +// (QueueLen > 0 → EnqueueMessage, not sendMessage) prevents FIFO inversion +// at the editor level. Here we verify the drain itself completes in order. +func TestDrain_FIFO_QueueNonEmptySlotMomentarilyIdle(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + const sid = "fifo-idle-window" + + pa := &pauseAgent{release: make(chan struct{})} + a, _ := newDrainApp(ctx, pa) + + // Enqueue M1 and start the drain worker. + a.queueMu.Lock() + a.queues[sid] = []QueuedMessage{{Text: "M1"}} + a.startDrainWorker(sid) + a.queueMu.Unlock() + + // Wait for the worker to start processing M1 (it's blocked in pause.Run). + waitFor(t, "M1 dequeued", func() bool { + pa.callMu.Lock() + defer pa.callMu.Unlock() + return len(pa.calls) == 1 + }) + + // While M1's Run is in-flight, enqueue M2 directly. + // At this point the queue is empty (M1 was dequeued) but a worker is + // active. EnqueueMessage will see an active worker and not start another. + a.EnqueueMessage(sid, QueuedMessage{Text: "M2"}) + + // Release M1's Run. + close(pa.release) + + waitFor(t, "both delivered", func() bool { + pa.callMu.Lock() + defer pa.callMu.Unlock() + return len(pa.calls) == 2 + }) + a.queueWg.Wait() + + pa.callMu.Lock() + got := pa.calls + pa.callMu.Unlock() + + if len(got) != 2 || got[0] != "M1" || got[1] != "M2" { + t.Errorf("FIFO violation in idle window: got %v, want [M1 M2]", got) + } +} + +// TestDrain_ErrorAttribution verifies the attributed error message wraps the +// original error and contains an attribution prefix. +func TestDrain_ErrorAttribution(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + apiErr := fmt.Errorf("context length exceeded") + ca := &countRunAgent{} + ca.preset = []runResult{{err: apiErr}} + + a, cap := newDrainApp(ctx, ca) + + sid := "attribution" + a.queueMu.Lock() + a.queues[sid] = []QueuedMessage{{Text: "m"}} + a.startDrainWorker(sid) + a.queueMu.Unlock() + + waitFor(t, "error event received", func() bool { + return len(cap.errors()) > 0 + }) + a.queueWg.Wait() + + err := cap.errors()[0] + if !errors.Is(err, apiErr) { + t.Errorf("error should wrap apiErr, got: %v", err) + } + const prefix = "queued message could not be delivered" + if errStr := err.Error(); len(errStr) < len(prefix) || errStr[:len(prefix)] != prefix { + t.Errorf("error attribution missing: %q", errStr) + } +} diff --git a/internal/app/queue.go b/internal/app/queue.go new file mode 100644 index 0000000000..757ab64f52 --- /dev/null +++ b/internal/app/queue.go @@ -0,0 +1,308 @@ +package app + +import ( + "context" + "errors" + "fmt" + "time" + + "github.com/opencode-ai/opencode/internal/llm/agent" + "github.com/opencode-ai/opencode/internal/logging" + "github.com/opencode-ai/opencode/internal/message" +) + +// QueuedMessage is a user message waiting to be delivered to the agent after +// the current run completes. It lives in memory only; it is never persisted to +// the database before delivery. Persisting before delivery would make +// agent-loop compaction (agent.go ~line 945) and the non-interactive reload +// (~line 1244) sweep the message into the in-flight run non-deterministically. +type QueuedMessage struct { + Text string + Attachments []message.Attachment +} + +// DrainEvent is emitted by the drain worker and EnqueueMessage to inform the +// TUI of queue-state changes. The TUI receives it via the registered +// DrainNotifier (see SetDrainNotifier). +type DrainEvent struct { + // SessionID identifies the affected session. + SessionID string + // QueueLen is the current queue length after the event. + QueueLen int + // Err is non-nil when the drain worker encountered a non-retryable error. + // When non-nil the drain worker has halted; remaining messages are still + // in the queue and visible via QueueLen. + Err error +} + +// SetDrainNotifier registers the callback that the drain worker calls to push +// DrainEvents to the TUI. Must be called once, from the TUI goroutine, before +// the first EnqueueMessage. +// +// The notifier is called synchronously and must be safe for concurrent use AND +// non-blocking. EnqueueMessage / DiscardQueue are invoked from the Bubble Tea +// update goroutine, so a notifier that blocks — notably a bare +// tea.Program.Send, whose channel is only read by the very event loop that is +// waiting for Update to return — deadlocks the whole TUI unrecoverably. Wrap +// delivery in a forwarder goroutine instead (see cmd.newDrainForwarder). +func (app *App) SetDrainNotifier(fn func(DrainEvent)) { + app.queueMu.Lock() + defer app.queueMu.Unlock() + app.drainNotify = fn +} + +// notify delivers a DrainEvent to the registered notifier (if any). Must be +// called without queueMu held. +func (app *App) notify(e DrainEvent) { + app.queueMu.Lock() + fn := app.drainNotify + app.queueMu.Unlock() + if fn != nil { + fn(e) + } +} + +// EnqueueMessage appends msg to sessionID's in-memory queue and starts a drain +// worker if one is not already running. Goroutine-safe. +func (app *App) EnqueueMessage(sessionID string, msg QueuedMessage) { + app.queueMu.Lock() + app.queues[sessionID] = append(app.queues[sessionID], msg) + qLen := len(app.queues[sessionID]) + if _, running := app.queueCancels[sessionID]; !running { + app.startDrainWorker(sessionID) + } + app.queueMu.Unlock() + app.notify(DrainEvent{SessionID: sessionID, QueueLen: qLen}) +} + +// DequeueMessage pops the head of sessionID's queue. Returns (msg, true) when +// a message was available, (zero, false) when the queue is empty. +// Goroutine-safe. +func (app *App) DequeueMessage(sessionID string) (QueuedMessage, bool) { + app.queueMu.Lock() + defer app.queueMu.Unlock() + q := app.queues[sessionID] + if len(q) == 0 { + return QueuedMessage{}, false + } + msg := q[0] + app.queues[sessionID] = q[1:] + return msg, true +} + +// dequeueOrRelease pops the head of sessionID's queue. When the queue is empty +// it ALSO deregisters the session's drain worker, atomically under queueMu. +// +// The atomicity matters: deciding "queue is empty, so exit" and "deregister the +// worker" in two separate critical sections is a check-then-act race. An +// EnqueueMessage landing between the two sees the still-registered cancel, +// declines to start a worker, and its message is then stranded in the queue +// with no worker to drain it (a lost wakeup — the message only moves when the +// user happens to submit again). +func (app *App) dequeueOrRelease(sessionID string) (QueuedMessage, bool) { + app.queueMu.Lock() + defer app.queueMu.Unlock() + q := app.queues[sessionID] + if len(q) == 0 { + delete(app.queueCancels, sessionID) + return QueuedMessage{}, false + } + msg := q[0] + app.queues[sessionID] = q[1:] + return msg, true +} + +// QueueLen returns the current queue depth for sessionID. Goroutine-safe. +func (app *App) QueueLen(sessionID string) int { + app.queueMu.Lock() + defer app.queueMu.Unlock() + return len(app.queues[sessionID]) +} + +// QueuedMessages returns a snapshot copy of sessionID's queue, head first. +// The returned slice is detached from the live queue, so the caller may read it +// while drain workers keep mutating. Goroutine-safe. +func (app *App) QueuedMessages(sessionID string) []QueuedMessage { + app.queueMu.Lock() + defer app.queueMu.Unlock() + q := app.queues[sessionID] + if len(q) == 0 { + return nil + } + out := make([]QueuedMessage, len(q)) + copy(out, q) + return out +} + +// DiscardQueue empties the queue for sessionID and notifies the TUI. +// Goroutine-safe. +func (app *App) DiscardQueue(sessionID string) { + app.queueMu.Lock() + app.queues[sessionID] = nil + app.queueMu.Unlock() + app.notify(DrainEvent{SessionID: sessionID, QueueLen: 0}) +} + +// prepend re-inserts msg at the head of sessionID's queue. Must be called +// under queueMu. +func (app *App) prepend(sessionID string, msg QueuedMessage) { + app.queues[sessionID] = append([]QueuedMessage{msg}, app.queues[sessionID]...) +} + +// startDrainWorker spawns a new drain goroutine for sessionID. Must be called +// under queueMu; the caller must have verified no worker is running. +func (app *App) startDrainWorker(sessionID string) { + ctx, cancel := context.WithCancel(app.ctx) + app.queueCancels[sessionID] = cancel + app.queueWg.Add(1) + go func() { + defer app.queueWg.Done() + app.drainLoop(ctx, sessionID) + }() +} + +// drainLoop is the body of the per-session drain worker goroutine. It dequeues +// messages one at a time and delivers each via agent.Run. It exits when: +// - the queue empties (normal completion); +// - a non-ErrSessionBusy error is returned by Run (halt after surfacing error); +// - its context is cancelled (app shutdown or explicit stop). +// +// ErrSessionBusy is the only swallowed error — the message is re-prepended and +// the worker backs off for 100 ms before retrying. All other errors are surfaced +// through the TUI via the registered DrainNotifier and halt the worker, leaving +// the remaining queue intact so the user can discard or allow a fresh drain. +func (app *App) drainLoop(ctx context.Context, sessionID string) { + const busyBackoff = 100 * time.Millisecond + + for { + // Respect context cancellation between attempts. + select { + case <-ctx.Done(): + return + default: + } + + // Dequeue-or-deregister is a single critical section: see + // dequeueOrRelease for why splitting it strands enqueued messages. + msg, ok := app.dequeueOrRelease(sessionID) + if !ok { + // Queue is empty and this worker is now deregistered. + app.notify(DrainEvent{SessionID: sessionID, QueueLen: 0}) + return + } + + ag := app.ActiveAgent() + if ag == nil { + // No agent available yet — re-prepend and wait briefly. + app.queueMu.Lock() + app.prepend(sessionID, msg) + app.queueMu.Unlock() + select { + case <-ctx.Done(): + return + case <-time.After(busyBackoff): + } + continue + } + + // Non-authoritative optimisation: skip a pointless Run call when the + // session is observably busy. The authoritative exclusivity mechanism + // is the atomic LoadOrStore inside RunWith; ErrSessionBusy from Run is + // the correct retry signal — not this check. + if ag.IsSessionBusy(sessionID) { + app.queueMu.Lock() + app.prepend(sessionID, msg) + app.queueMu.Unlock() + select { + case <-ctx.Done(): + return + case <-time.After(busyBackoff): + } + continue + } + + events, err := ag.Run(ctx, sessionID, msg.Text, 0, msg.Attachments...) + if err != nil { + if errors.Is(err, agent.ErrSessionBusy) { + // Lost the acquire race — re-prepend and back off. + app.queueMu.Lock() + app.prepend(sessionID, msg) + app.queueMu.Unlock() + select { + case <-ctx.Done(): + return + case <-time.After(busyBackoff): + } + continue + } + + // Non-retryable error: surface with attribution, halt worker, + // preserve remaining queue (including the failed message at head). + app.queueMu.Lock() + app.prepend(sessionID, msg) + remaining := len(app.queues[sessionID]) + delete(app.queueCancels, sessionID) + app.queueMu.Unlock() + app.notify(DrainEvent{ + SessionID: sessionID, + QueueLen: remaining, + Err: fmt.Errorf("queued message could not be delivered: %w", err), + }) + logging.Warn("drain worker halted after error", + "session", sessionID, "error", err, "remaining", remaining) + return + } + + // Drain the events channel so the agent's panic-recover path can + // complete and release the busy lock. Select on ctx.Done() so a + // shutdown signal is not missed while the channel is open. + drainEvents: + for { + select { + case _, ok := <-events: + if !ok { + break drainEvents + } + case <-ctx.Done(): + return + } + } + + qLen := app.QueueLen(sessionID) + app.notify(DrainEvent{SessionID: sessionID, QueueLen: qLen}) + } +} + +// ShutdownQueues cancels all live drain workers and blocks until they exit. +// Called from App.Shutdown to prevent goroutine leaks. +func (app *App) ShutdownQueues() { + app.queueMu.Lock() + for _, cancel := range app.queueCancels { + cancel() + } + app.queueMu.Unlock() + app.queueWg.Wait() +} + +// EnqueueForTest appends msg to sessionID's queue WITHOUT starting a drain +// worker. It is only for use in tests and must not be called in production +// code: UI tests need a queue that stays put, whereas EnqueueMessage spawns a +// worker that immediately starts popping (and, with no agent wired, +// re-prepending) the head. +func (app *App) EnqueueForTest(sessionID string, msg QueuedMessage) { + app.queueMu.Lock() + defer app.queueMu.Unlock() + app.queues[sessionID] = append(app.queues[sessionID], msg) +} + +// NewForTest creates a minimal App for unit tests. The returned App has the +// queue subsystem and active agent initialized; all other services are nil. +// It is only for use in tests and must not be called in production code. +func NewForTest(ctx context.Context, ag agent.Service) *App { + return &App{ + ctx: ctx, + queues: make(map[string][]QueuedMessage), + queueCancels: make(map[string]context.CancelFunc), + activeAgent: ag, + } +} diff --git a/internal/app/queue_test.go b/internal/app/queue_test.go new file mode 100644 index 0000000000..f136a4182e --- /dev/null +++ b/internal/app/queue_test.go @@ -0,0 +1,284 @@ +package app + +import ( + "context" + "sync" + "testing" + "time" + + "github.com/opencode-ai/opencode/internal/message" +) + +// newTestApp returns a minimal App for queue tests. It has an explicit context +// so tests can cancel it to stop drain workers. +func newTestApp(ctx context.Context) *App { + return &App{ + ctx: ctx, + queues: make(map[string][]QueuedMessage), + queueCancels: make(map[string]context.CancelFunc), + } +} + +// TestQueue_EnqueueDequeue_FIFO asserts that DequeueMessage returns messages in +// enqueue order. +func TestQueue_EnqueueDequeue_FIFO(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + a := newTestApp(ctx) + + // Manually enqueue without starting the drain worker (we hold queueMu). + a.queueMu.Lock() + a.queues["s1"] = append(a.queues["s1"], QueuedMessage{Text: "first"}) + a.queues["s1"] = append(a.queues["s1"], QueuedMessage{Text: "second"}) + a.queues["s1"] = append(a.queues["s1"], QueuedMessage{Text: "third"}) + a.queueMu.Unlock() + + m1, ok1 := a.DequeueMessage("s1") + m2, ok2 := a.DequeueMessage("s1") + m3, ok3 := a.DequeueMessage("s1") + _, ok4 := a.DequeueMessage("s1") // empty + + if !ok1 || m1.Text != "first" { + t.Errorf("want first got %q (ok=%v)", m1.Text, ok1) + } + if !ok2 || m2.Text != "second" { + t.Errorf("want second got %q (ok=%v)", m2.Text, ok2) + } + if !ok3 || m3.Text != "third" { + t.Errorf("want third got %q (ok=%v)", m3.Text, ok3) + } + if ok4 { + t.Error("expected empty after three dequeues") + } +} + +// TestQueue_QueueLen tracks correctly. +func TestQueue_QueueLen(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + a := newTestApp(ctx) + + if got := a.QueueLen("s1"); got != 0 { + t.Fatalf("initial QueueLen = %d, want 0", got) + } + + a.queueMu.Lock() + a.queues["s1"] = append(a.queues["s1"], QueuedMessage{Text: "a"}) + a.queues["s1"] = append(a.queues["s1"], QueuedMessage{Text: "b"}) + a.queueMu.Unlock() + + if got := a.QueueLen("s1"); got != 2 { + t.Fatalf("QueueLen after 2 enqueues = %d, want 2", got) + } + + a.DequeueMessage("s1") + if got := a.QueueLen("s1"); got != 1 { + t.Fatalf("QueueLen after 1 dequeue = %d, want 1", got) + } +} + +// TestQueue_QueuedMessages returns a detached, head-first snapshot. +func TestQueue_QueuedMessages(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + a := newTestApp(ctx) + + if got := a.QueuedMessages("s1"); got != nil { + t.Fatalf("empty queue snapshot = %v, want nil", got) + } + + a.queueMu.Lock() + a.queues["s1"] = append(a.queues["s1"], + QueuedMessage{Text: "first"}, + QueuedMessage{Text: "second"}, + ) + a.queueMu.Unlock() + + snap := a.QueuedMessages("s1") + if len(snap) != 2 || snap[0].Text != "first" || snap[1].Text != "second" { + t.Fatalf("snapshot = %+v, want [first second]", snap) + } + + // The snapshot must survive further queue mutation — the TUI renders it + // while drain workers keep popping the head. + a.DequeueMessage("s1") + if len(snap) != 2 || snap[0].Text != "first" { + t.Errorf("snapshot mutated by dequeue: %+v", snap) + } + if got := a.QueuedMessages("s1"); len(got) != 1 || got[0].Text != "second" { + t.Errorf("snapshot after dequeue = %+v, want [second]", got) + } +} + +// TestQueue_DiscardQueue empties the queue for the target session only. +func TestQueue_DiscardQueue(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + a := newTestApp(ctx) + + a.queueMu.Lock() + a.queues["s1"] = []QueuedMessage{{Text: "x"}, {Text: "y"}} + a.queues["s2"] = []QueuedMessage{{Text: "z"}} + a.queueMu.Unlock() + + a.DiscardQueue("s1") + + if got := a.QueueLen("s1"); got != 0 { + t.Errorf("s1 QueueLen after discard = %d, want 0", got) + } + if got := a.QueueLen("s2"); got != 1 { + t.Errorf("s2 QueueLen should be untouched, got %d", got) + } +} + +// TestQueue_Concurrent_NoRace enqueues and dequeues from many goroutines to +// detect data races under go test -race. +func TestQueue_Concurrent_NoRace(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + a := newTestApp(ctx) + + const goroutines = 16 + const perGoroutine = 20 + sid := "race-session" + + var wg sync.WaitGroup + for i := 0; i < goroutines; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for j := 0; j < perGoroutine; j++ { + a.queueMu.Lock() + a.queues[sid] = append(a.queues[sid], QueuedMessage{Text: "data"}) + a.queueMu.Unlock() + a.DequeueMessage(sid) + _ = a.QueueLen(sid) + } + }() + } + wg.Wait() +} + +// TestQueue_Attachments verifies that Attachments are preserved through +// enqueue/dequeue. +func TestQueue_Attachments(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + a := newTestApp(ctx) + + att := message.Attachment{FileName: "test.png"} + a.queueMu.Lock() + a.queues["s1"] = append(a.queues["s1"], QueuedMessage{ + Text: "with attachment", + Attachments: []message.Attachment{att}, + }) + a.queueMu.Unlock() + + msg, ok := a.DequeueMessage("s1") + if !ok { + t.Fatal("expected a message") + } + if msg.Text != "with attachment" { + t.Errorf("unexpected text %q", msg.Text) + } + if len(msg.Attachments) != 1 || msg.Attachments[0].FileName != "test.png" { + t.Errorf("attachments not preserved: %+v", msg.Attachments) + } +} + +// TestQueue_ShutdownQueues_cancelsWorkers verifies that ShutdownQueues cancels +// drain workers without blocking indefinitely (no goroutine leak). We start a +// real drain worker and cancel it via ShutdownQueues. +func TestQueue_ShutdownQueues_cancelsWorkers(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + a := newTestApp(ctx) + + sid := "shutdown-test" + + // Start a drain worker manually without actual agent (it will spin on the + // busy-check path since ActiveAgent() returns nil). We just want to confirm + // the worker terminates when ShutdownQueues is called. + a.queueMu.Lock() + a.queues[sid] = []QueuedMessage{{Text: "pending"}} + a.startDrainWorker(sid) + a.queueMu.Unlock() + + // Give the worker a moment to start and enter its wait loop. + time.Sleep(10 * time.Millisecond) + + done := make(chan struct{}) + go func() { + a.ShutdownQueues() + close(done) + }() + + select { + case <-done: + // OK — workers exited promptly. + case <-time.After(2 * time.Second): + t.Fatal("ShutdownQueues did not return within 2s (goroutine leak?)") + } +} + +// TestQueue_DequeueOrRelease_AtomicDeregistration locks in the invariant that +// makes the drain worker's exit safe: the worker is deregistered from +// queueCancels in the SAME critical section that observes the empty queue. +// +// Splitting the two (dequeue returns false, then a second lock deletes the +// cancel) lets an EnqueueMessage land in between: it sees the still-registered +// worker, declines to start one, and its message is stranded in the queue with +// nothing to drain it until the user submits again. +func TestQueue_DequeueOrRelease_AtomicDeregistration(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + a := newTestApp(ctx) + + a.queueMu.Lock() + a.queues["s1"] = []QueuedMessage{{Text: "a"}} + a.queueCancels["s1"] = func() {} + a.queueMu.Unlock() + + msg, ok := a.dequeueOrRelease("s1") + if !ok || msg.Text != "a" { + t.Fatalf("dequeueOrRelease = (%q, %v), want (\"a\", true)", msg.Text, ok) + } + a.queueMu.Lock() + _, registered := a.queueCancels["s1"] + a.queueMu.Unlock() + if !registered { + t.Error("worker deregistered while it still holds a message") + } + + if _, ok := a.dequeueOrRelease("s1"); ok { + t.Fatal("dequeueOrRelease returned a message from an empty queue") + } + a.queueMu.Lock() + _, registered = a.queueCancels["s1"] + a.queueMu.Unlock() + if registered { + t.Error("worker not deregistered after observing an empty queue") + } +} + +// TestQueue_EnqueueDuringWorkerExit_AlwaysDelivered stresses the enqueue-while- +// worker-exiting window: every enqueued message must eventually be delivered, +// and the queue must never come to rest non-empty with no worker running. +func TestQueue_EnqueueDuringWorkerExit_AlwaysDelivered(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + ag := &countRunAgent{} + a := newTestApp(ctx) + a.activeAgent = ag + + const total = 300 + for i := 0; i < total; i++ { + a.EnqueueMessage("s1", QueuedMessage{Text: "m"}) + time.Sleep(50 * time.Microsecond) + } + + waitFor(t, "all queued messages delivered", func() bool { + return int(ag.count.Load()) == total && a.QueueLen("s1") == 0 + }) +} diff --git a/internal/bridge/bridge.go b/internal/bridge/bridge.go index 1fc6557d46..64db261945 100644 --- a/internal/bridge/bridge.go +++ b/internal/bridge/bridge.go @@ -23,6 +23,7 @@ package bridge import ( "context" + "fmt" "strings" ) @@ -361,3 +362,51 @@ type Adapter interface { type JobScopedAdapter interface { SetJobID(jobID string) } + +// QueueAckToken is the platform-native message identifier returned by +// QueuedAcknowledger.SendQueuedAck. It is an opaque string that is passed +// back to UpdateQueuedAck to edit the message in-place. +// +// Platform conventions: +// - Telegram: string-encoded int message ID ("1234") +// - Slack: channelID + "\x00" + ts (both needed to call UpdateMessageContext) +// - Mattermost: post ID (26-char string) +type QueueAckToken = string + +// QueuedAcknowledger is an optional interface adapters may implement to +// send and in-place-edit queued-acknowledgement messages. Callers check +// with the ok-pattern and skip silently when not satisfied: +// +// if ack, ok := adapter.(bridge.QueuedAcknowledger); ok { ... } +// +// UpdateQueuedAck position conventions: +// - position > 0: queued; text reflects the ordinal (1 = "you're next") +// - position == 0: sentinel for "run has started — resolve the ack" +// (edit to "▶ Processing…" or equivalent) +// +// All three production adapters implement this interface. Test doubles +// that do not implement it behave as if QueueAcknowledgementsEnabled == false. +type QueuedAcknowledger interface { + // SendQueuedAck sends the initial queued-ack message to peer and + // returns a token for subsequent in-place edits. position is 1-based + // (1 = "you're next"). + SendQueuedAck(ctx context.Context, peer PeerRef, position int) (QueueAckToken, error) + + // UpdateQueuedAck edits the message identified by token in-place. + // Pass position == 0 to resolve the ack (run started). + UpdateQueuedAck(ctx context.Context, peer PeerRef, token QueueAckToken, position int) error +} + +// QueueAckText returns the chat text for a queued-acknowledgement message. +// position is 1-based: 1 means the message is next (only the current in-flight +// run is blocking it). position == 0 is the resolved sentinel; use ResolvedAckText. +func QueueAckText(position int) string { + if position <= 1 { + return "⏳ Your message is queued. I'll respond as soon as the current run finishes." + } + return fmt.Sprintf("⏳ Your message is queued — %d message(s) ahead. I'll respond once they finish.", position-1) +} + +// ResolvedAckText is the text for the final in-place edit when the queued +// message's agent run begins. Displayed briefly before the agent's reply arrives. +const ResolvedAckText = "▶ Processing your message now…" diff --git a/internal/bridge/config.go b/internal/bridge/config.go index ff54c648da..23078c4342 100644 --- a/internal/bridge/config.go +++ b/internal/bridge/config.go @@ -67,6 +67,15 @@ type Config struct { // 0 → use the built-in default (QuestionNudgeDefaultMax) // <0 → unlimited (bounded in practice by the job deadline) QuestionNudgeMax int `json:"questionNudgeMax,omitempty"` + + // QueueAcknowledgementsEnabled controls whether the bridge sends an + // in-place-editable acknowledgement to a sender whose inbound message + // is enqueued behind an in-flight agent run. When true, the sender + // receives a "⏳ queued" note that is edited as the queue drains and + // resolved to "▶ Processing…" when the run starts. When false (the + // default), messages queue silently as before. Adapters that do not + // implement bridge.QueuedAcknowledger are silently skipped. + QueueAcknowledgementsEnabled bool `json:"queueAcknowledgementsEnabled,omitempty"` } // ChannelsConfig holds per-platform channel sections. diff --git a/internal/bridge/mattermost/adapter.go b/internal/bridge/mattermost/adapter.go index 140caa7366..9d5e54bcee 100644 --- a/internal/bridge/mattermost/adapter.go +++ b/internal/bridge/mattermost/adapter.go @@ -739,3 +739,47 @@ func truncateRunes(s string, maxRunes int) string { } return s } + +// Compile-time assertion: Adapter implements bridge.QueuedAcknowledger. +var _ bridge.QueuedAcknowledger = (*Adapter)(nil) + +// SendQueuedAck creates a queued-acknowledgement post in the peer's channel +// and returns the post ID as the token for subsequent in-place edits. +func (a *Adapter) SendQueuedAck(ctx context.Context, peer bridge.PeerRef, position int) (bridge.QueueAckToken, error) { + parsed := ParsePeerID(peer.PeerID) + if parsed.ChannelID == "" { + return "", fmt.Errorf("mattermost: SendQueuedAck: invalid peer %q", peer.PeerID) + } + post, err := a.client.CreatePost(ctx, CreatePostInput{ + ChannelID: parsed.ChannelID, + Message: bridge.QueueAckText(position), + RootID: parsed.RootPostID, + }) + if err != nil { + return "", fmt.Errorf("mattermost: SendQueuedAck: %w", err) + } + return post.ID, nil +} + +// UpdateQueuedAck edits the queued-ack post identified by token in-place. +// token is the post ID returned by SendQueuedAck. Pass position == 0 to +// resolve the ack ("▶ Processing…"). +func (a *Adapter) UpdateQueuedAck(ctx context.Context, _ bridge.PeerRef, token bridge.QueueAckToken, position int) error { + if token == "" { + return fmt.Errorf("mattermost: UpdateQueuedAck: empty token") + } + var text string + if position == 0 { + text = bridge.ResolvedAckText + } else { + text = bridge.QueueAckText(position) + } + _, err := a.client.UpdatePost(ctx, UpdatePostInput{ + PostID: token, + Message: text, + }) + if err != nil { + return fmt.Errorf("mattermost: UpdateQueuedAck: %w", err) + } + return nil +} diff --git a/internal/bridge/mattermost/adapter_test.go b/internal/bridge/mattermost/adapter_test.go index 1b87d981e0..45fe3f9433 100644 --- a/internal/bridge/mattermost/adapter_test.go +++ b/internal/bridge/mattermost/adapter_test.go @@ -58,6 +58,7 @@ type mockServer struct { // Captured request bodies for inspection by tests. createPostCalls []CreatePostInput + updatePosts []UpdatePostInput uploadCalls []FileUpload directCalls []string } @@ -81,6 +82,7 @@ func newMockServer(t *testing.T, bot User) *mockServer { mux.HandleFunc("/api/v4/users/me", m.handleGetMe) mux.HandleFunc("/api/v4/users/", m.handleUsersTyping) mux.HandleFunc("/api/v4/posts", m.handlePosts) + mux.HandleFunc("/api/v4/posts/", m.handlePostsSlash) mux.HandleFunc("/api/v4/files", m.handleFilesUpload) mux.HandleFunc("/api/v4/files/", m.handleFileDownload) mux.HandleFunc("/api/v4/channels/direct", m.handleChannelsDirect) @@ -139,6 +141,25 @@ func (m *mockServer) handlePosts(w http.ResponseWriter, r *http.Request) { _ = json.NewEncoder(w).Encode(m.createPostResp(captured)) } +func (m *mockServer) handlePostsSlash(w http.ResponseWriter, r *http.Request) { + // PUT /api/v4/posts/{postID} — used by client.UpdatePost + if r.Method != http.MethodPut { + w.WriteHeader(http.StatusMethodNotAllowed) + return + } + postID := strings.TrimPrefix(r.URL.Path, "/api/v4/posts/") + body, _ := io.ReadAll(r.Body) + var raw map[string]any + _ = json.Unmarshal(body, &raw) + msg, _ := raw["message"].(string) + in := UpdatePostInput{PostID: postID, Message: msg} + m.mu.Lock() + m.updatePosts = append(m.updatePosts, in) + m.mu.Unlock() + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(Post{ID: postID, Message: in.Message}) +} + func (m *mockServer) handleFilesUpload(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodPost { w.WriteHeader(http.StatusMethodNotAllowed) diff --git a/internal/bridge/mattermost/queue_ack_test.go b/internal/bridge/mattermost/queue_ack_test.go new file mode 100644 index 0000000000..5ec06f73a7 --- /dev/null +++ b/internal/bridge/mattermost/queue_ack_test.go @@ -0,0 +1,97 @@ +package mattermost + +import ( + "context" + "strings" + "testing" + + "github.com/opencode-ai/opencode/internal/bridge" +) + +func testMattermostAdapter(t *testing.T) (*Adapter, *mockServer) { + t.Helper() + bot := User{ID: "bot1", Username: "testbot"} + mock := newMockServer(t, bot) + a, _, stop := startAdapter(t, + Identity{ID: "local", ServerURL: mock.URL(), AccessToken: "tok"}, + mock, 4) + t.Cleanup(stop) + return a, mock +} + +// TestSendQueuedAck_Mattermost verifies SendQueuedAck creates a post and +// returns the post ID as the token. +func TestSendQueuedAck_Mattermost(t *testing.T) { + t.Parallel() + a, mock := testMattermostAdapter(t) + + peer := bridge.PeerRef{Channel: "mattermost", Identity: "local", PeerID: "channel1"} + tok, err := a.SendQueuedAck(context.Background(), peer, 1) + if err != nil { + t.Fatalf("SendQueuedAck: %v", err) + } + if tok == "" { + t.Fatal("SendQueuedAck returned empty token") + } + // mock createPostResp returns ID "new_post" + if tok != "new_post" { + t.Errorf("token = %q, want \"new_post\"", tok) + } + + mock.mu.Lock() + posts := mock.createPostCalls + mock.mu.Unlock() + if len(posts) == 0 { + t.Fatal("CreatePost was not called") + } + if !strings.Contains(posts[len(posts)-1].Message, "⏳") { + t.Errorf("ack text missing ⏳ glyph: %q", posts[len(posts)-1].Message) + } +} + +// TestUpdateQueuedAck_Mattermost verifies UpdateQueuedAck calls UpdatePost +// and resolves with the "▶" text when position == 0. +func TestUpdateQueuedAck_Mattermost(t *testing.T) { + t.Parallel() + a, mock := testMattermostAdapter(t) + + peer := bridge.PeerRef{Channel: "mattermost", Identity: "local", PeerID: "channel1"} + tok, err := a.SendQueuedAck(context.Background(), peer, 1) + if err != nil { + t.Fatalf("SendQueuedAck: %v", err) + } + + if err := a.UpdateQueuedAck(context.Background(), peer, tok, 1); err != nil { + t.Fatalf("UpdateQueuedAck position=1: %v", err) + } + if err := a.UpdateQueuedAck(context.Background(), peer, tok, 0); err != nil { + t.Fatalf("UpdateQueuedAck position=0 (resolve): %v", err) + } + + mock.mu.Lock() + updates := mock.updatePosts + mock.mu.Unlock() + + if len(updates) < 2 { + t.Fatalf("UpdatePost called %d times, want ≥2", len(updates)) + } + last := updates[len(updates)-1] + if !strings.Contains(last.Message, "▶") { + t.Errorf("resolve update text missing ▶ glyph: %q", last.Message) + } + if last.PostID != tok { + t.Errorf("UpdatePost PostID = %q, want token %q", last.PostID, tok) + } +} + +// TestUpdateQueuedAck_Mattermost_EmptyToken rejects an empty token. +func TestUpdateQueuedAck_Mattermost_EmptyToken(t *testing.T) { + t.Parallel() + a, _ := testMattermostAdapter(t) + err := a.UpdateQueuedAck(context.Background(), + bridge.PeerRef{Channel: "mattermost", Identity: "local", PeerID: "channel1"}, + "", 1) + if err == nil { + t.Error("expected error for empty token") + } +} diff --git a/internal/bridge/service/commands.go b/internal/bridge/service/commands.go index 2846ace3b7..54d61e869b 100644 --- a/internal/bridge/service/commands.go +++ b/internal/bridge/service/commands.go @@ -353,6 +353,13 @@ func (s *Service) cmdSession(ctx context.Context, in bridge.Inbound) *bridge.Com // seeing the pre-switch answer and jobs on the re-bound session // deferred 60s/tick forever (until process restart). s.invalidateSessionScopeCaches() + // A reviewer is now attached to the target session, so it is no longer + // unattended: a `question` raised there must reach this chat instead of + // being auto-answered. Matters when the target is a former flow-step + // session (flow.Service.runStep marks every step session unattended). + if s.app != nil && s.app.Permissions != nil { + s.app.Permissions.RemoveUnattendedSession(target.ID) + } // Ensure the dispatcher for the new session is up so the next // inbound routes without a cold-start delay. The old session's // dispatcher is left alone — closeDispatcherIfEmpty would tear it diff --git a/internal/bridge/service/dispatch.go b/internal/bridge/service/dispatch.go index df9e335a8c..de6149c1fb 100644 --- a/internal/bridge/service/dispatch.go +++ b/internal/bridge/service/dispatch.go @@ -20,9 +20,8 @@ import ( // Per-session dispatch channel capacities, per the chat-bridge spec: // // - inbound: 16, NEVER drop. Reviewers' messages MUST NOT be lost. -// Pushers (adapter-side per-peer goroutines) block when the queue -// fills — chat platforms have their own buffering that absorbs the -// stall. +// When the channel is full, messages spill into the per-session +// overflow slice so the shared runInboundLoop is never stalled. // - parts: 64, drop-oldest. Part-event transitions can collapse // ("completed" supersedes "running" supersedes "pending"). Drops are // rate-limited to one warn log per session per minute. @@ -37,8 +36,26 @@ const ( // same iteration of the event loop, so the queued events are // already in flight; we only need to give the broker time to push. partsDrainGrace = 100 * time.Millisecond + + // busyRetryBackoff is the sleep between ErrSessionBusy retries. + // Matches the TUI drain worker's precedent (app/queue.go busyBackoff). + busyRetryBackoff = 100 * time.Millisecond ) +// busyRetryBudget is the maximum time handleInbound will retry agent.Run on +// ErrSessionBusy before re-queuing the message via the overflow path. +// Cross-actor holders (flow steps, cron sentinels, task auto-resume) can hold a +// session for several minutes; 5 minutes gives them a generous window before +// the message is re-queued for another attempt. Content is never discarded. +// A variable rather than a const so tests can shrink it (see busyAckThreshold). +var busyRetryBudget = 5 * time.Minute + +// busyAckThreshold is the minimum duration of ErrSessionBusy retrying before +// a queued-acknowledgement is sent to the peer (Decision 3: 2-second short-wait +// threshold). Hardcoded for v1; exported as a variable so tests can override it +// without an N×100 ms spin wait. +var busyAckThreshold = 2 * time.Second + // toolErrorPreviewRunes caps the failure reason appended to a ✗ tool // line. Tool updates are compact by design (name + id + duration only); // a failed call is the one case that carries body text, because an @@ -65,8 +82,16 @@ type sessionDispatch struct { inbound chan bridge.Inbound parts chan pubsub.Event[message.PartEvent] + // mu guards overflowLog, overflow, and the non-blocking push/drain + // interlock. MUST NOT be held across I/O or across calls that acquire + // another lock. mu sync.Mutex overflowLog time.Time + // overflow holds inbound messages that could not fit into d.inbound + // when the channel was full (non-starvation fix — see pushInbound). + // Drained back into d.inbound by drainOverflowToInbound after each + // handleInbound call. Guarded by mu. + overflow []bridge.Inbound stop atomic.Bool @@ -85,6 +110,16 @@ type sessionDispatch struct { // sweep removes stale entries when a call never produces a paired // result (rare — usually a cancelled cycle). toolCallStart sync.Map // map[string]int64 + + // liveAcks remembers the outstanding queued-ack token per peer so it + // survives a busy-retry-budget re-queue. handleInbound's ack state is a + // local and budget expiry returns from handleInbound — without this the + // next 5-minute cycle would SEND a brand-new "⏳ queued" message instead + // of editing the existing one, leaving one orphaned, never-resolved ack + // per cycle in the reviewer's chat. Keys: peerAckKey(peer); values: + // bridge.QueueAckToken. Entries are removed when the ack is resolved + // (run started) or when an edit fails (message gone — send a fresh one). + liveAcks sync.Map // map[string]bridge.QueueAckToken } // newSessionDispatch constructs and launches the per-session dispatcher @@ -121,6 +156,10 @@ func (s *Service) newSessionDispatch(sessionID string) *sessionDispatch { // session at a time), so this loop only processes one inbound at a // time. Parts events are handled in parallel by runParts so they don't // have to wait for the run to finish. +// +// After each handleInbound, overflow items are drained back into +// d.inbound (FIFO) so they are processed before any newly-arriving +// messages from runInboundLoop. func (d *sessionDispatch) run(ctx context.Context) { for { select { @@ -134,6 +173,33 @@ func (d *sessionDispatch) run(ctx context.Context) { return } d.handleInbound(ctx, in) + d.drainOverflowToInbound() + } + } +} + +// drainOverflowToInbound transfers overflow items into d.inbound under +// mu so the transfer is atomic with concurrent pushInbound calls from +// runInboundLoop. Called by run() after each handleInbound. +// +// FIFO ordering: overflow items are older than any items that arrive +// concurrently from runInboundLoop. Transferring them into d.inbound +// (a FIFO channel) while holding mu ensures new arrivals see the channel +// full and go to overflow AFTER the existing overflow items — so read +// order is: +// +// [items already in d.inbound] → [drained overflow] → [new arrivals] +func (d *sessionDispatch) drainOverflowToInbound() { + d.mu.Lock() + defer d.mu.Unlock() + for len(d.overflow) > 0 { + select { + case d.inbound <- d.overflow[0]: + d.overflow = d.overflow[1:] + default: + // Channel still full; remaining overflow items stay and + // will be drained on the next handleInbound cycle. + return } } } @@ -188,11 +254,14 @@ func (d *sessionDispatch) handleInbound(ctx context.Context, in bridge.Inbound) } }() - // Named `ag`, not `agent`: the local must not shadow the agent package, - // which the ErrSessionBusy branch below needs. + // Named `ag`, not `agent`: the local must not shadow the agent package. ag := d.svc.app.ActiveAgent() if ag == nil { logging.Warn("bridge: no active agent; dropping inbound", "session", d.sessionID) + d.svc.replyToPeer(ctx, in.Peer, + "bridge: this session has no active agent — your message could not be processed. "+ + "Please try again once the agent is available.", + false, d.sessionID) return } @@ -215,12 +284,71 @@ func (d *sessionDispatch) handleInbound(ctx context.Context, in bridge.Inbound) partsSub := d.svc.app.Messages.SubscribeParts(partsCtx) atts := translateAttachments(in.Attachments) - runCh, err := ag.Run(ctx, d.sessionID, in.Text, 0, atts...) - if err != nil { - logging.Warn("bridge: agent.Run failed", "session", d.sessionID, "err", err) - d.svc.replyToPeer(ctx, in.Peer, runFailureMessage(err, d.sessionID), false, d.sessionID) - return + + // Bounded retry for ErrSessionBusy: the session-run ledger is + // process-global (session-run-exclusivity spec). Cross-actor holders + // — a flow step's own agent, a cron sentinel, a task auto-resume — + // make agent.Run return ErrSessionBusy here. The bridge's single- + // dispatcher serialization prevents bridge-vs-bridge collisions but + // cannot prevent cross-actor ones. Retry with 100 ms backoff for up + // to 5 minutes; on budget expiry, re-queue the message via the + // overflow path so content is NEVER discarded. + // + // Queued-ack lifecycle (Decision 2 + Decision 3): + // - A 2-second short-wait timer arms on the first ErrSessionBusy. + // - If the timer fires while still retrying AND acks are enabled, + // the peer receives a "⏳ queued" message (SendQueuedAck). + // - The ack is updated in-place ONLY when the reported position + // actually changes (UpdateQueuedAck). Editing on every 100 ms retry + // would issue thousands of identical edits per queued message, which + // burns the platforms' edit rate limits and makes Telegram reject the + // call outright ("message is not modified"). + // - When Run succeeds, the ack is resolved to "▶ Processing…" + // (UpdateQueuedAck, position=0). + deadline := time.Now().Add(busyRetryBudget) + ackThreshold := time.Now().Add(busyAckThreshold) + ack := queueAckState{lastPosition: -1} + if tok, ok := d.liveAcks.Load(peerAckKey(in.Peer)); ok { + // A previous retry cycle for this peer already has an ack message in + // chat (busy-retry budget expired and the inbound was re-queued). + // Reuse it so the peer sees one ack that keeps updating. + ack.token, _ = tok.(bridge.QueueAckToken) + } + var runCh <-chan agent.AgentEvent + for { + var err error + runCh, err = ag.Run(ctx, d.sessionID, in.Text, 0, atts...) + if err == nil { + break + } + if !errors.Is(err, agent.ErrSessionBusy) { + // Non-busy error: the run never started, so the ack must NOT be + // resolved to "▶ Processing…" — that would contradict the failure + // reply sent immediately after. Leave the "⏳ queued" text in place. + logging.Warn("bridge: agent.Run failed", "session", d.sessionID, "err", err) + d.svc.replyToPeer(ctx, in.Peer, runFailureMessage(err, d.sessionID), false, d.sessionID) + return + } + // ErrSessionBusy from a cross-actor holder. Check budget. + if time.Now().After(deadline) { + // Do NOT resolve the ack here: the message is being re-queued, not + // processed. Resolving would tell the peer "▶ Processing your + // message now…" while it goes back to the tail of the retry cycle. + logging.Warn("bridge: ErrSessionBusy budget expired; re-queuing inbound", + "session", d.sessionID) + d.pushInbound(in) + return + } + // Check / send / update the queued-ack. + d.tickQueueAck(ctx, in.Peer, &ack, &ackThreshold) + select { + case <-ctx.Done(): + return + case <-time.After(busyRetryBackoff): + } } + // Run succeeded — resolve the ack before starting the run. + d.resolveQueueAck(ctx, in.Peer, ack.token) // Fan part events into d.parts for outbound surface delivery (typing, // tool-update prints). Filter to this session's parts; broker is @@ -242,32 +370,105 @@ func (d *sessionDispatch) handleInbound(ctx context.Context, in bridge.Inbound) } // runFailureMessage builds the chat-surface text for an agent.Run that failed -// to start. A stuck session must be observable to the reviewer instead of -// silently swallowing messages, but the advice has to match the cause. -// -// ErrSessionBusy is split out deliberately. The per-session dispatch goroutine -// serializes this package's only Run callsite, so the bridge cannot collide -// with itself — but the session-run ledger is process-global (see -// internal/llm/agent/session_locks.go), so a run started elsewhere in the -// process (a flow step's own agent instance) makes the session read as busy -// here. Interactive flow steps never reach handleInbound at all — inbound.go -// buffers inbound for sessions carrying the interactive marker — but if that -// guard ever regresses, the generic advice would be actively harmful: -// aborting the session cancels the live step, which Cancel's cross-instance -// fallback now actually reaches. So busy gets "wait and resend", not "abort". +// to start with a non-busy error. ErrSessionBusy is handled by the retry +// loop in handleInbound and never reaches this function. func runFailureMessage(err error, sessionID string) string { - if errors.Is(err, agent.ErrSessionBusy) { - return "bridge: this session already has a run in flight elsewhere " + - "(it may be owned by a flow step). Your message was not delivered — " + - "please resend once the current run finishes. Do NOT abort the session: " + - "that would cancel the in-flight run." - } // Cap the detail leaked to chat to the public-facing fields. return "bridge: agent run failed (" + err.Error() + "). " + "If this keeps happening, use /reset in chat to clear the session " + "or POST /session/" + sessionID + "/abort to release the busy lock." } +// peerAckKey is the liveAcks map key for a peer. Channel+identity+peerID is +// the same tuple bindings are keyed on, so two identities in the same channel +// never share an ack slot. +func peerAckKey(p bridge.PeerRef) string { + return p.Channel + "|" + p.Identity + "|" + p.PeerID +} + +// queueAckState is handleInbound's local queued-ack bookkeeping: the platform +// token for in-place edits and the position last rendered into it. lastPosition +// starts at -1 ("nothing rendered yet") so position 0 can never be mistaken for +// an already-rendered value. +type queueAckState struct { + token bridge.QueueAckToken + lastPosition int +} + +// tickQueueAck manages the queued-ack lifecycle on each ErrSessionBusy retry +// cycle. On first call after the short-wait threshold (busyAckThreshold), it +// sends the initial "⏳ queued" message if acks are enabled. On subsequent +// calls it updates the ack in-place ONLY when the position it would render has +// changed — the retry loop ticks every 100 ms, so editing unconditionally would +// issue up to 3000 identical edits per queued message (rate-limit exhaustion on +// Slack/Mattermost, and a hard "message is not modified" error on Telegram). +// +// ack is a pointer to handleInbound's local ack state. +// threshold is a pointer to the firing time. +func (d *sessionDispatch) tickQueueAck(ctx context.Context, peer bridge.PeerRef, ack *queueAckState, threshold *time.Time) { + if d.svc.cfg == nil || !d.svc.cfg.QueueAcknowledgementsEnabled { + return + } + adapter := d.svc.Adapter(peer.Channel, peer.Identity) + acker, ok := adapter.(bridge.QueuedAcknowledger) + if !ok { + return + } + // position = 1 means the message is next-in-line once the current + // cross-actor holder releases the slot. + position := 1 + len(d.inbound) + // Initial send: fires when threshold has elapsed AND we don't yet have a token. + if ack.token == "" { + if time.Now().Before(*threshold) { + return + } + tok, err := acker.SendQueuedAck(ctx, peer, position) + if err != nil { + logging.Info("bridge: SendQueuedAck failed", "session", d.sessionID, "err", err) + return + } + ack.token = tok + ack.lastPosition = position + d.liveAcks.Store(peerAckKey(peer), tok) + return + } + // Update in-place only when the rendered text would actually change. + if position == ack.lastPosition { + return + } + if err := acker.UpdateQueuedAck(ctx, peer, ack.token, position); err != nil { + logging.Info("bridge: UpdateQueuedAck failed", "session", d.sessionID, "err", err) + // The ack message may be gone (deleted by the user, or a token from a + // previous cycle that is no longer editable). Forget it so the next + // tick sends a fresh one rather than editing into the void forever. + d.liveAcks.Delete(peerAckKey(peer)) + ack.token = "" + return + } + ack.lastPosition = position +} + +// resolveQueueAck edits the ack message to "▶ Processing…" (position == 0). +// A no-op when ackToken is empty or acks are disabled. +func (d *sessionDispatch) resolveQueueAck(ctx context.Context, peer bridge.PeerRef, ackToken bridge.QueueAckToken) { + if ackToken == "" { + return + } + // The run is starting: this ack is done, whatever the edit's outcome. + d.liveAcks.Delete(peerAckKey(peer)) + if d.svc.cfg == nil || !d.svc.cfg.QueueAcknowledgementsEnabled { + return + } + adapter := d.svc.Adapter(peer.Channel, peer.Identity) + acker, ok := adapter.(bridge.QueuedAcknowledger) + if !ok { + return + } + if err := acker.UpdateQueuedAck(ctx, peer, ackToken, 0); err != nil { + logging.Info("bridge: resolveQueueAck failed", "session", d.sessionID, "err", err) + } +} + // drainParts forwards parts for this session AND any of its descendant // (subagent) sessions from the broker subscription to d.parts. Returns // when partsCtx is cancelled (set by handleInbound after agent.Run @@ -892,24 +1093,75 @@ func translateAttachments(in []bridge.Attachment) []message.Attachment { return out } -// closeOnce marks the dispatcher as stopped and drains its channels. -// Caller MUST hold s.dispatchMu. +// close marks the dispatcher as stopped, drains queued messages for +// shutdown-loss logging, and closes d.inbound. Caller MUST hold +// s.dispatchMu. +// +// Draining AND the close itself are protected by d.mu so they serialize +// against concurrent drainOverflowToInbound / pushInbound calls: pushInbound +// re-checks d.stop under the same mutex, so no send can land on the closed +// channel (a send on a closed channel panics, taking down the dispatcher +// goroutine). Items that run() has already received (one possible item after +// stop is set) are not logged — that is an accepted race at shutdown per +// Decision 5. func (d *sessionDispatch) close() { if !d.stop.CompareAndSwap(false, true) { return } + // Collect any queued inbound messages for WARN logging before closing. + d.mu.Lock() + lost := make([]bridge.Inbound, 0, len(d.overflow)+len(d.inbound)) + lost = append(lost, d.overflow...) + d.overflow = nil +drainLoop: + for { + select { + case item := <-d.inbound: + lost = append(lost, item) + default: + break drainLoop + } + } close(d.inbound) + d.mu.Unlock() + + for _, item := range lost { + logging.Warn("bridge: shutdown lost queued inbound", + "session", d.sessionID, + "peer", item.Peer.PeerID) + } + if n := len(lost); n > 0 { + logging.Warn("bridge: shutdown dropped queued messages", + "session", d.sessionID, + "count", n) + } } -// pushInbound enqueues an inbound message onto the dispatcher's inbound -// channel. Blocks when the channel is full (the spec's "back-pressure -// adapter instead of drop" semantics). Returns ctx.Err() if ctx is -// cancelled while waiting for capacity. -func (d *sessionDispatch) pushInbound(ctx context.Context, in bridge.Inbound) error { +// pushInbound enqueues an inbound message. The push is NON-BLOCKING: +// if d.inbound is full, the message is appended to the per-session +// overflow slice instead of blocking the caller. This prevents a single +// session from stalling the shared runInboundLoop (cross-session +// non-starvation fix). +// +// Both the channel send and the overflow append are done under d.mu to +// serialize with drainOverflowToInbound calls in run(), preserving +// per-session FIFO order (overflow items are served before new arrivals). +// The same mutex makes the d.stop re-check safe: close() sets stop and closes +// d.inbound under d.mu, so a push that observes !stop can never send on a +// closed channel. +func (d *sessionDispatch) pushInbound(in bridge.Inbound) { + d.mu.Lock() + defer d.mu.Unlock() + if d.stop.Load() { + // Dispatcher already torn down (unbind or shutdown). Dropping is + // audible rather than silent, and never a panic. + logging.Warn("bridge: dropped inbound for stopped dispatcher", + "session", d.sessionID, "peer", in.Peer.PeerID) + return + } select { case d.inbound <- in: - return nil - case <-ctx.Done(): - return ctx.Err() + default: + d.overflow = append(d.overflow, in) } } diff --git a/internal/bridge/service/dispatch_ack_test.go b/internal/bridge/service/dispatch_ack_test.go new file mode 100644 index 0000000000..86a112de3e --- /dev/null +++ b/internal/bridge/service/dispatch_ack_test.go @@ -0,0 +1,473 @@ +package service + +// Tests for Part 2 of bridge-queue-visibility-and-loss-paths: +// - QueuedAcknowledger ack lifecycle in handleInbound (task 8.5) +// - Config-gate proof: ack suppressed when disabled, sent when enabled +// - TestBufferInbound_NoDrainWithoutQuestion (task 9.2) + +import ( + "context" + "errors" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/opencode-ai/opencode/internal/app" + "github.com/opencode-ai/opencode/internal/bridge" + "github.com/opencode-ai/opencode/internal/bridge/store" + "github.com/opencode-ai/opencode/internal/config" + agentpkg "github.com/opencode-ai/opencode/internal/llm/agent" + "github.com/opencode-ai/opencode/internal/message" + "github.com/opencode-ai/opencode/internal/question" +) + +// --------------------------------------------------------------------------- +// ackStubAdapter — a stubAdapter that also implements bridge.QueuedAcknowledger +// --------------------------------------------------------------------------- + +type ackStubAdapter struct { + stubAdapter + + mu sync.Mutex + sendCalls []ackSendCall + updateCalls []ackUpdateCall +} + +type ackSendCall struct { + Peer bridge.PeerRef + Position int +} +type ackUpdateCall struct { + Peer bridge.PeerRef + Token bridge.QueueAckToken + Position int +} + +func newAckStubAdapter(channel, identity string) *ackStubAdapter { + return &ackStubAdapter{ + stubAdapter: *newStubAdapter(channel, identity), + } +} + +func (a *ackStubAdapter) SendQueuedAck(ctx context.Context, peer bridge.PeerRef, position int) (bridge.QueueAckToken, error) { + a.mu.Lock() + a.sendCalls = append(a.sendCalls, ackSendCall{Peer: peer, Position: position}) + a.mu.Unlock() + return "token-1", nil +} + +func (a *ackStubAdapter) UpdateQueuedAck(_ context.Context, peer bridge.PeerRef, token bridge.QueueAckToken, position int) error { + a.mu.Lock() + a.updateCalls = append(a.updateCalls, ackUpdateCall{Peer: peer, Token: token, Position: position}) + a.mu.Unlock() + return nil +} + +func (a *ackStubAdapter) AckSendCount() int { + a.mu.Lock() + defer a.mu.Unlock() + return len(a.sendCalls) +} + +func (a *ackStubAdapter) AckUpdateCalls() []ackUpdateCall { + a.mu.Lock() + defer a.mu.Unlock() + out := make([]ackUpdateCall, len(a.updateCalls)) + copy(out, a.updateCalls) + return out +} + +// newAckDispatchTestSvc builds a service+dispatcher suitable for ack lifecycle tests. +// acks: whether QueueAcknowledgementsEnabled is true. +func newAckDispatchTestSvc(t *testing.T, ag agentpkg.Service, acksEnabled bool) (*Service, *ackStubAdapter) { + t.Helper() + svc, _ := newOrchestratorForTest(t) + svc.cfg = &bridge.Config{QueueAcknowledgementsEnabled: acksEnabled} + svc.app = &app.App{ + Messages: &stubMessageSvc{}, + PrimaryAgents: map[config.AgentName]agentpkg.Service{config.AgentCoder: ag}, + PrimaryAgentKeys: []config.AgentName{config.AgentCoder}, + } + ad := newAckStubAdapter("slack", "default") + svc.adapters[adapterKey("slack", "default")] = ad + if _, err := svc.store.UpsertBinding(context.Background(), store.Binding{ + ProjectID: "proj", Channel: "slack", IdentityID: "default", + PeerID: "D1", SessionID: "S1", + }); err != nil { + t.Fatalf("UpsertBinding: %v", err) + } + return svc, ad +} + +// --------------------------------------------------------------------------- +// Task 8.5: TestHandleInbound_QueuedAckLifecycle +// --------------------------------------------------------------------------- + +// TestHandleInbound_QueuedAckLifecycle is the end-to-end ack lifecycle test: +// +// (a) SendQueuedAck is called exactly once, after busyAckThreshold elapses +// (b) UpdateQueuedAck is NOT called again while the reported position is +// unchanged — the retry loop ticks every 100 ms, and re-editing the same +// text thousands of times exhausts platform edit rate limits (Telegram +// rejects an unchanged edit outright with "message is not modified") +// (c) UpdateQueuedAck is called once more with position==0 (resolve) when Run succeeds +// +// busyAckThreshold is set to 0 so the test runs deterministically without sleeping. +func TestHandleInbound_QueuedAckLifecycle(t *testing.T) { + // busyAckThreshold = 0 means the ack fires immediately on the first retry. + old := busyAckThreshold + busyAckThreshold = 0 + defer func() { busyAckThreshold = old }() + + // N=3 busy calls before success; gives 3 retries after the first call. + const N = 3 + errs := make([]error, N) + for i := range errs { + errs[i] = agentpkg.ErrSessionBusy + } + ag := &busyRetryStubAgent{runErrors: errs} + svc, ad := newAckDispatchTestSvc(t, ag, true) // acks enabled + + in := testInbound("ack-lifecycle-message") + d := newBareDispatch(svc, "S1") + d.handleInbound(context.Background(), in) + + // (a) SendQueuedAck called exactly once. + if got := ad.AckSendCount(); got != 1 { + t.Errorf("SendQueuedAck called %d times, want 1", got) + } + + updates := ad.AckUpdateCalls() + // (b) The position never changes in this scenario (nothing else is queued), + // so the only update must be the resolve. No redundant same-position edits. + if len(updates) != 1 { + t.Errorf("UpdateQueuedAck called %d times, want 1 (resolve only); got %v", + len(updates), updates) + } + + // (c) Final UpdateQueuedAck must have position==0 (resolve sentinel). + if len(updates) == 0 { + t.Fatal("UpdateQueuedAck never called") + } + last := updates[len(updates)-1] + if last.Position != 0 { + t.Errorf("last UpdateQueuedAck position = %d, want 0 (resolve)", last.Position) + } + if last.Token != "token-1" { + t.Errorf("last UpdateQueuedAck token = %q, want \"token-1\"", last.Token) + } +} + +// --------------------------------------------------------------------------- +// Config-gate proof: acks suppressed when disabled, sent when enabled +// --------------------------------------------------------------------------- + +// TestQueuedAck_ConfigGate_Disabled proves that with QueueAcknowledgementsEnabled=false +// no SendQueuedAck or UpdateQueuedAck is ever called, even when ErrSessionBusy is returned. +func TestQueuedAck_ConfigGate_Disabled(t *testing.T) { + old := busyAckThreshold + busyAckThreshold = 0 + defer func() { busyAckThreshold = old }() + + errs := []error{agentpkg.ErrSessionBusy, agentpkg.ErrSessionBusy} + ag := &busyRetryStubAgent{runErrors: errs} + svc, ad := newAckDispatchTestSvc(t, ag, false) // acks DISABLED + + d := newBareDispatch(svc, "S1") + d.handleInbound(context.Background(), testInbound("gate-off")) + + if got := ad.AckSendCount(); got != 0 { + t.Errorf("GATE DISABLED: SendQueuedAck called %d times, want 0", got) + } + if updates := ad.AckUpdateCalls(); len(updates) != 0 { + t.Errorf("GATE DISABLED: UpdateQueuedAck called %d times, want 0", len(updates)) + } +} + +// TestQueuedAck_ConfigGate_Enabled proves the ack IS sent when the gate is on. +func TestQueuedAck_ConfigGate_Enabled(t *testing.T) { + old := busyAckThreshold + busyAckThreshold = 0 + defer func() { busyAckThreshold = old }() + + errs := []error{agentpkg.ErrSessionBusy} + ag := &busyRetryStubAgent{runErrors: errs} + svc, ad := newAckDispatchTestSvc(t, ag, true) // acks ENABLED + + d := newBareDispatch(svc, "S1") + d.handleInbound(context.Background(), testInbound("gate-on")) + + if got := ad.AckSendCount(); got == 0 { + t.Error("GATE ENABLED: SendQueuedAck was NOT called — ack was not sent") + } + updates := ad.AckUpdateCalls() + if len(updates) == 0 { + t.Error("GATE ENABLED: UpdateQueuedAck never called (resolve never happened)") + } + last := updates[len(updates)-1] + if last.Position != 0 { + t.Errorf("GATE ENABLED: last UpdateQueuedAck position = %d, want 0 (resolve)", last.Position) + } +} + +// --------------------------------------------------------------------------- +// Task 9.2: TestBufferInbound_NoDrainWithoutQuestion +// --------------------------------------------------------------------------- + +// TestBufferInbound_NoDrainWithoutQuestion verifies that when a question arrives +// for an interactive session that already has buffered messages, handleNewRequest +// auto-answers the question from the HEAD of the buffer (FIFO), leaving the +// rest in the buffer, and does NOT fan the question out to peers. +func TestBufferInbound_NoDrainWithoutQuestion(t *testing.T) { + // Not parallel: calls newOrchestratorForTest which acquires gooseSerialMu, + // and also drives question.Service goroutines. Running parallel increases + // race-detector false-positive risk for the goose migrations mutex. + svc, _ := newOrchestratorForTest(t) + svc.app = &app.App{ + Questions: question.NewService(), + } + r := newBufferRouter(svc) + svc.questionRouter = r + + ad := newStubAdapter("slack", "default") + svc.adapters[adapterKey("slack", "default")] = ad + if _, err := svc.store.UpsertBinding(context.Background(), store.Binding{ + ProjectID: "proj", Channel: "slack", IdentityID: "default", + PeerID: "D1", SessionID: "S1", + }); err != nil { + t.Fatalf("UpsertBinding: %v", err) + } + + // Buffer 3 messages in FIFO order: "first", "second", "third". + ctx := context.Background() + r.BufferInbound(ctx, "S1", bridge.Inbound{ + Peer: bridge.PeerRef{Channel: "slack", Identity: "default", PeerID: "D1"}, + Text: "first", + }) + r.BufferInbound(ctx, "S1", bridge.Inbound{ + Peer: bridge.PeerRef{Channel: "slack", Identity: "default", PeerID: "D1"}, + Text: "second", + }) + r.BufferInbound(ctx, "S1", bridge.Inbound{ + Peer: bridge.PeerRef{Channel: "slack", Identity: "default", PeerID: "D1"}, + Text: "third", + }) + if got := r.bufferedLen("S1"); got != 3 { + t.Fatalf("setup: buffered %d, want 3", got) + } + + // Subscribe so we can capture the question request. + sub := svc.app.Questions.Subscribe(ctx) + + // Start an Ask in the background. + type askResult struct { + answers [][]string + err error + } + resultCh := make(chan askResult, 1) + var runCalls atomic.Int32 + go func() { + runCalls.Add(1) + ans, err := svc.app.Questions.Ask(ctx, "S1", []question.Prompt{{ + Question: "Approve?", + Options: []question.Option{{Label: "first"}, {Label: "other"}}, + }}) + resultCh <- askResult{ans, err} + }() + + // Capture the CreatedEvent and drive handleNewRequest directly. + select { + case ev := <-sub: + r.handleNewRequest(ctx, ev.Payload) + case <-time.After(2 * time.Second): + t.Fatal("no question CreatedEvent observed") + } + + select { + case res := <-resultCh: + if res.err != nil { + t.Fatalf("Ask returned error: %v", res.err) + } + // The HEAD of the buffer ("first") must have answered the question. + if len(res.answers) == 0 || len(res.answers[0]) == 0 || res.answers[0][0] != "first" { + t.Errorf("Ask answered with %v, want [[first]] (FIFO head)", res.answers) + } + case <-time.After(2 * time.Second): + t.Fatal("Ask never returned") + } + + // Two messages remain in buffer ("second", "third"). + if got := r.bufferedLen("S1"); got != 2 { + t.Errorf("buffer len = %d after auto-answer, want 2", got) + } + // Question was NOT fanned out to the adapter. + if sends := ad.Sends(); len(sends) != 0 { + t.Errorf("question fanned out to adapter (%d sends); auto-answer should suppress fan-out", len(sends)) + } +} + +// --------------------------------------------------------------------------- +// Ack edit economy + honest ack state on the non-delivery paths +// --------------------------------------------------------------------------- + +// ackPositionAgent returns ErrSessionBusy for a preset number of calls and, on +// the call at index growAt, pushes an extra inbound into the dispatcher so the +// reported queue position changes mid-retry. +type ackPositionAgent struct { + busyRetryStubAgent + d *sessionDispatch + growAt int +} + +func (a *ackPositionAgent) Run( + ctx context.Context, sid, content string, mt int, atts ...message.Attachment, +) (<-chan agentpkg.AgentEvent, error) { + a.mu.Lock() + idx := a.calls + a.mu.Unlock() + if idx == a.growAt && a.d != nil { + a.d.pushInbound(testInbound("filler")) + } + return a.busyRetryStubAgent.Run(ctx, sid, content, mt, atts...) +} + +// TestQueuedAck_UpdatesOnlyOnPositionChange proves the ack is edited when the +// position actually changes, and not on the retries where it does not. Without +// the position-change gate the 100 ms retry loop issues an identical edit per +// tick — thousands per queued message against the platform's edit rate limit. +func TestQueuedAck_UpdatesOnlyOnPositionChange(t *testing.T) { + old := busyAckThreshold + busyAckThreshold = 0 + defer func() { busyAckThreshold = old }() + + errs := make([]error, 4) + for i := range errs { + errs[i] = agentpkg.ErrSessionBusy + } + ag := &ackPositionAgent{ + busyRetryStubAgent: busyRetryStubAgent{runErrors: errs}, + growAt: 2, + } + svc, ad := newAckDispatchTestSvc(t, ag, true) + d := newBareDispatch(svc, "S1") + ag.d = d + + d.handleInbound(context.Background(), testInbound("position-change")) + + if got := ad.AckSendCount(); got != 1 { + t.Errorf("SendQueuedAck called %d times, want 1", got) + } + updates := ad.AckUpdateCalls() + // Expected: one update for the position change (2), then the resolve (0). + if len(updates) != 2 { + t.Fatalf("UpdateQueuedAck calls = %v, want exactly 2 (position change + resolve)", updates) + } + if updates[0].Position != 2 { + t.Errorf("first update position = %d, want 2", updates[0].Position) + } + if updates[1].Position != 0 { + t.Errorf("second update position = %d, want 0 (resolve)", updates[1].Position) + } +} + +// TestQueuedAck_NotResolvedOnRunFailure asserts the ack is NOT resolved to +// "▶ Processing your message now…" when the run never starts: resolving would +// directly contradict the failure reply sent immediately afterwards. +func TestQueuedAck_NotResolvedOnRunFailure(t *testing.T) { + old := busyAckThreshold + busyAckThreshold = 0 + defer func() { busyAckThreshold = old }() + + ag := &busyRetryStubAgent{runErrors: []error{ + agentpkg.ErrSessionBusy, + errors.New("provider exploded"), + }} + svc, ad := newAckDispatchTestSvc(t, ag, true) + d := newBareDispatch(svc, "S1") + + d.handleInbound(context.Background(), testInbound("run-failure")) + + if got := ad.AckSendCount(); got != 1 { + t.Fatalf("SendQueuedAck called %d times, want 1", got) + } + for _, u := range ad.AckUpdateCalls() { + if u.Position == 0 { + t.Errorf("ack resolved (position 0) even though the run never started: %v", u) + } + } +} + +// TestQueuedAck_NotResolvedOnBudgetExpiry asserts the ack is NOT resolved when +// the busy-retry budget expires: the message is re-queued for another attempt, +// so telling the peer "▶ Processing your message now…" would be false. +func TestQueuedAck_NotResolvedOnBudgetExpiry(t *testing.T) { + oldThreshold := busyAckThreshold + busyAckThreshold = 0 + defer func() { busyAckThreshold = oldThreshold }() + + errs := make([]error, 64) + for i := range errs { + errs[i] = agentpkg.ErrSessionBusy + } + ag := &busyRetryStubAgent{runErrors: errs} + svc, ad := newAckDispatchTestSvc(t, ag, true) + d := newBareDispatch(svc, "S1") + + // Shrink the retry budget so it expires after the ack has been sent but + // before Run ever succeeds. + oldBudget := busyRetryBudget + busyRetryBudget = 150 * time.Millisecond + defer func() { busyRetryBudget = oldBudget }() + + d.handleInbound(context.Background(), testInbound("budget-expiry")) + + for _, u := range ad.AckUpdateCalls() { + if u.Position == 0 { + t.Errorf("ack resolved (position 0) on budget expiry — message was re-queued, not processed: %v", u) + } + } + // The message must be re-queued, never dropped. + if len(d.inbound) == 0 && len(d.overflow) == 0 { + t.Error("inbound was neither re-queued into d.inbound nor overflow") + } +} + +// TestQueuedAck_SurvivesRequeueCycle asserts that when the busy-retry budget +// expires and the inbound is re-queued, the NEXT cycle edits the ack message +// that is already in chat instead of sending a second one. Without the +// dispatcher-level token memo, a session held for hours accumulates one +// orphaned, never-resolved "⏳ queued" message per 5-minute cycle. +func TestQueuedAck_SurvivesRequeueCycle(t *testing.T) { + oldThreshold := busyAckThreshold + busyAckThreshold = 0 + defer func() { busyAckThreshold = oldThreshold }() + oldBudget := busyRetryBudget + busyRetryBudget = 150 * time.Millisecond + defer func() { busyRetryBudget = oldBudget }() + + errs := make([]error, 64) + for i := range errs { + errs[i] = agentpkg.ErrSessionBusy + } + ag := &busyRetryStubAgent{runErrors: errs} + svc, ad := newAckDispatchTestSvc(t, ag, true) + d := newBareDispatch(svc, "S1") + + in := testInbound("requeue-ack") + // Cycle 1: ack sent, budget expires, inbound re-queued. + d.handleInbound(context.Background(), in) + if got := ad.AckSendCount(); got != 1 { + t.Fatalf("cycle 1: SendQueuedAck called %d times, want 1", got) + } + // Cycle 2: same peer, still busy — must reuse the existing ack. + d.handleInbound(context.Background(), in) + if got := ad.AckSendCount(); got != 1 { + t.Errorf("cycle 2: SendQueuedAck called %d times total, want 1 (ack must be reused, not re-sent)", got) + } + for _, u := range ad.AckUpdateCalls() { + if u.Token != "token-1" { + t.Errorf("update used token %q, want the original \"token-1\"", u.Token) + } + } +} diff --git a/internal/bridge/service/dispatch_busy_retry_test.go b/internal/bridge/service/dispatch_busy_retry_test.go new file mode 100644 index 0000000000..a59e1df64d --- /dev/null +++ b/internal/bridge/service/dispatch_busy_retry_test.go @@ -0,0 +1,545 @@ +package service + +// Tests for Part 1 of bridge-queue-visibility-and-loss-paths: +// - ErrSessionBusy retry + content preservation (tasks 1.3, 1.4) +// - Cross-session non-starvation via non-blocking push (task 1.6e) +// - Session serialization invariant (task 1.7) +// - Nil-agent reply (task 2.2) +// - Interactive-buffer eviction notification (task 3.2) +// - Shutdown WARN log (task 4.2) + +import ( + "context" + "log/slog" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/opencode-ai/opencode/internal/app" + "github.com/opencode-ai/opencode/internal/bridge" + "github.com/opencode-ai/opencode/internal/bridge/store" + "github.com/opencode-ai/opencode/internal/config" + agentpkg "github.com/opencode-ai/opencode/internal/llm/agent" + "github.com/opencode-ai/opencode/internal/message" + "github.com/opencode-ai/opencode/internal/pubsub" +) + +// --------------------------------------------------------------------------- +// Stub helpers +// --------------------------------------------------------------------------- + +// busyRetryStubAgent implements agent.Service for dispatch tests. +// Only Run is implemented; all other methods panic via nil embedding. +// runErrors[i] is returned as the error for the i-th Run call (nil = success). +type busyRetryStubAgent struct { + agentpkg.Service // nil — other methods are never called in these tests + + mu sync.Mutex + runErrors []error + calls int + lastText string + + // slowDuration, if > 0, makes Run block for that duration before returning. + slowDuration time.Duration + // maxConcurrent tracks the peak number of concurrent Run invocations. + maxConcurrent atomic.Int32 + activeCalls atomic.Int32 +} + +func (a *busyRetryStubAgent) Run( + _ context.Context, _, content string, _ int, _ ...message.Attachment, +) (<-chan agentpkg.AgentEvent, error) { + // Track concurrency + cur := a.activeCalls.Add(1) + defer a.activeCalls.Add(-1) + for { + old := a.maxConcurrent.Load() + if cur <= old || a.maxConcurrent.CompareAndSwap(old, cur) { + break + } + } + + a.mu.Lock() + idx := a.calls + a.calls++ + a.lastText = content + errs := a.runErrors + a.mu.Unlock() + + if idx < len(errs) && errs[idx] != nil { + // Busy or other error — return immediately without the slow wait + return nil, errs[idx] + } + + if a.slowDuration > 0 { + time.Sleep(a.slowDuration) + } + + ch := make(chan agentpkg.AgentEvent, 1) + ch <- agentpkg.AgentEvent{Type: agentpkg.AgentEventTypeResponse} + close(ch) + return ch, nil +} + +func (a *busyRetryStubAgent) runCallCount() int { + a.mu.Lock() + defer a.mu.Unlock() + return a.calls +} + +// stubMessageSvc implements message.Service. Only SubscribeParts is wired; +// all other methods panic via nil embedding. +type stubMessageSvc struct { + message.Service +} + +func (s *stubMessageSvc) SubscribeParts(ctx context.Context) <-chan pubsub.Event[message.PartEvent] { + ch := make(chan pubsub.Event[message.PartEvent]) + go func() { + <-ctx.Done() + close(ch) + }() + return ch +} + +// newDispatchTestSvc builds a minimal Service suitable for handleInbound tests. +// The stub agent and adapter are wired; a binding for peer "D1"/session "S1" +// is inserted. The run() and runParts() goroutines are NOT started — callers +// invoke handleInbound directly. +func newDispatchTestSvc(t *testing.T, ag agentpkg.Service) (*Service, *stubAdapter) { + t.Helper() + svc, _ := newOrchestratorForTest(t) + svc.app = &app.App{ + Messages: &stubMessageSvc{}, + PrimaryAgents: map[config.AgentName]agentpkg.Service{config.AgentCoder: ag}, + PrimaryAgentKeys: []config.AgentName{config.AgentCoder}, + } + ad := newStubAdapter("slack", "default") + svc.adapters[adapterKey("slack", "default")] = ad + if _, err := svc.store.UpsertBinding(context.Background(), store.Binding{ + ProjectID: "proj", Channel: "slack", IdentityID: "default", + PeerID: "D1", SessionID: "S1", + }); err != nil { + t.Fatalf("UpsertBinding: %v", err) + } + return svc, ad +} + +// newBareDispatch constructs a sessionDispatch without starting goroutines. +// Useful for testing pushInbound, close(), and overflow directly. +func newBareDispatch(svc *Service, sessionID string) *sessionDispatch { + return &sessionDispatch{ + svc: svc, + sessionID: sessionID, + inbound: make(chan bridge.Inbound, dispatchInboundCap), + parts: make(chan pubsub.Event[message.PartEvent], dispatchPartsCap), + } +} + +func testInbound(text string) bridge.Inbound { + return bridge.Inbound{ + Peer: bridge.PeerRef{Channel: "slack", Identity: "default", PeerID: "D1"}, + Text: text, + } +} + +// --------------------------------------------------------------------------- +// Task 1.4: TestHandleInbound_BusyRetryPreservesContent +// --------------------------------------------------------------------------- + +// TestHandleInbound_BusyRetryPreservesContent verifies that when agent.Run +// returns ErrSessionBusy N times and then succeeds: +// +// (a) Run is called N+1 times with the same inbound text +// (b) the run-failure reply path is NOT taken (no replyToPeer for busy) +// (c) the inbound text is intact on the final successful Run call +// +// To prove the test bites: with the OLD blocking pushInbound+immediate discard +// behavior, Run would only be called once (returning ErrSessionBusy) and +// the failure reply ("please resend") would be sent. The retry loop fixes this. +func TestHandleInbound_BusyRetryPreservesContent(t *testing.T) { + // N=2 busy calls before success; test takes ~200ms (2 * busyRetryBackoff) + const N = 2 + errs := make([]error, N) + for i := range errs { + errs[i] = agentpkg.ErrSessionBusy + } + ag := &busyRetryStubAgent{runErrors: errs} + svc, ad := newDispatchTestSvc(t, ag) + + in := testInbound("keep this content please") + d := newBareDispatch(svc, "S1") + d.handleInbound(context.Background(), in) + + // (a) Run called N+1 times + if got := ag.runCallCount(); got != N+1 { + t.Errorf("Run called %d times, want %d (N=%d busy + 1 success)", got, N+1, N) + } + // (b) No failure reply was sent via the adapter (busy errors go to retry, + // not to runFailureMessage) + sends := ad.Sends() + for _, s := range sends { + if strings.Contains(s.Text, "resend") || strings.Contains(s.Text, "not delivered") { + t.Errorf("failure reply was sent: %q — ErrSessionBusy should be retried, not reported", s.Text) + } + } + // (c) Last Run call received the original content + ag.mu.Lock() + lastText := ag.lastText + ag.mu.Unlock() + if lastText != in.Text { + t.Errorf("last Run text = %q, want %q", lastText, in.Text) + } +} + +// --------------------------------------------------------------------------- +// Task 1.6e: TestDispatch_NonBlockingPush_NoStarvation +// --------------------------------------------------------------------------- + +// TestDispatch_NonBlockingPush_NoStarvation verifies that when session A's +// d.inbound channel is full, a push to A goes to overflow (non-blocking) and +// does NOT delay a concurrent push to session B. +// +// Proof of bite: with the OLD blocking pushInbound, filling A's channel and +// then calling pushInbound(mA) would block indefinitely, never reaching B's +// push. This test would hang (or timeout). With the fix, it completes in O(1). +func TestDispatch_NonBlockingPush_NoStarvation(t *testing.T) { + t.Parallel() + svc, _ := newOrchestratorForTest(t) + + dispA := newBareDispatch(svc, "A") + dispB := newBareDispatch(svc, "B") + + // Fill session A's inbound to cap. + for i := 0; i < dispatchInboundCap; i++ { + dispA.inbound <- testInbound("fill") + } + if len(dispA.inbound) != dispatchInboundCap { + t.Fatalf("setup: A.inbound should be full, got len=%d", len(dispA.inbound)) + } + + mA := testInbound("message for A") + mB := testInbound("message for B") + + // Push to A (full channel) — must be non-blocking + done := make(chan struct{}) + go func() { + dispA.pushInbound(mA) + dispB.pushInbound(mB) + close(done) + }() + + select { + case <-done: + // OK — both pushes completed without blocking + case <-time.After(2 * time.Second): + t.Fatal("pushInbound blocked — full-channel push stalled the shared loop (non-starvation bug)") + } + + // mA must be in overflow (channel was full) + dispA.mu.Lock() + overflowLen := len(dispA.overflow) + var overflowText string + if overflowLen > 0 { + overflowText = dispA.overflow[0].Text + } + dispA.mu.Unlock() + if overflowLen != 1 { + t.Errorf("A overflow len = %d, want 1", overflowLen) + } + if overflowText != mA.Text { + t.Errorf("A overflow[0].Text = %q, want %q", overflowText, mA.Text) + } + + // mB must be in B's inbound channel (not blocked) + if got := len(dispB.inbound); got != 1 { + t.Errorf("B.inbound len = %d, want 1 — session B was not dispatched immediately", got) + } +} + +// --------------------------------------------------------------------------- +// Overflow FIFO: drainOverflowToInbound delivers in arrival order +// --------------------------------------------------------------------------- + +// TestOverflowFIFO verifies that overflow items are transferred to d.inbound +// in arrival order and appear before any newly-arriving items. +func TestOverflowFIFO(t *testing.T) { + t.Parallel() + svc, _ := newOrchestratorForTest(t) + d := newBareDispatch(svc, "S1") + + // Fill inbound to cap. + for i := 0; i < dispatchInboundCap; i++ { + d.inbound <- testInbound("channel") + } + + // Push 3 overflow items. + ov1 := testInbound("overflow-1") + ov2 := testInbound("overflow-2") + ov3 := testInbound("overflow-3") + d.pushInbound(ov1) + d.pushInbound(ov2) + d.pushInbound(ov3) + + d.mu.Lock() + if len(d.overflow) != 3 { + d.mu.Unlock() + t.Fatalf("overflow len = %d, want 3", len(d.overflow)) + } + d.mu.Unlock() + + // Drain one "channel" item (simulating handleInbound consuming it). + <-d.inbound + + // Drain overflow into inbound. + d.drainOverflowToInbound() + + // The inbound channel should now have (cap-1) old items + 1 overflow item. + // Read past the old items. + for i := 0; i < dispatchInboundCap-1; i++ { + item := <-d.inbound + if item.Text != "channel" { + t.Fatalf("expected \"channel\" item at position %d, got %q", i, item.Text) + } + } + + // Next item must be overflow-1 (FIFO). + got := <-d.inbound + if got.Text != ov1.Text { + t.Errorf("FIFO violated: got %q after old items, want %q", got.Text, ov1.Text) + } + + // overflow-2 and overflow-3 remain in overflow (channel was re-filled). + d.mu.Lock() + remaining := len(d.overflow) + d.mu.Unlock() + if remaining != 2 { + t.Errorf("overflow remaining = %d, want 2 (ov2 and ov3)", remaining) + } +} + +// --------------------------------------------------------------------------- +// Task 1.7: TestSessionSerializationInvariant +// --------------------------------------------------------------------------- + +// TestSessionSerializationInvariant verifies that the dispatcher processes +// inbound messages serially: the second agent.Run does not start until the +// first completes. Uses a slow mock with a controlled delay. +func TestSessionSerializationInvariant(t *testing.T) { + t.Parallel() + + const runDelay = 50 * time.Millisecond + ag := &busyRetryStubAgent{slowDuration: runDelay} + svc, _ := newDispatchTestSvc(t, ag) + + // Start the dispatcher goroutines via the service's supervised launcher. + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + svc.ctx = ctx + svc.cancel = cancel + + d := svc.newSessionDispatch("S1") + t.Cleanup(func() { d.close() }) + + m1 := testInbound("first") + m2 := testInbound("second") + d.pushInbound(m1) + d.pushInbound(m2) + + // Wait for both messages to be processed. + deadline := time.Now().Add(3 * time.Second) + for time.Now().Before(deadline) { + if ag.runCallCount() >= 2 { + break + } + time.Sleep(5 * time.Millisecond) + } + if ag.runCallCount() < 2 { + t.Fatalf("only %d Run calls observed, want 2", ag.runCallCount()) + } + + // Assert no concurrent Runs occurred. + if mc := ag.maxConcurrent.Load(); mc > 1 { + t.Errorf("maxConcurrent = %d, want 1 (serialization invariant violated)", mc) + } +} + +// --------------------------------------------------------------------------- +// Task 2.2: TestHandleInbound_NilAgentReplies +// --------------------------------------------------------------------------- + +// TestHandleInbound_NilAgentReplies verifies that when ActiveAgent() returns +// nil, handleInbound sends a reply to the sender's peer and does not panic. +func TestHandleInbound_NilAgentReplies(t *testing.T) { + t.Parallel() + svc, _ := newOrchestratorForTest(t) + + // Set up app with no agent (PrimaryAgents map is nil → ActiveAgent returns nil). + svc.app = &app.App{ + Messages: &stubMessageSvc{}, + PrimaryAgents: nil, + PrimaryAgentKeys: []config.AgentName{config.AgentCoder}, + } + + ad := newStubAdapter("slack", "default") + svc.adapters[adapterKey("slack", "default")] = ad + + d := newBareDispatch(svc, "S1") + in := testInbound("hello") + + // Must not panic. + d.handleInbound(context.Background(), in) + + // A reply MUST have been sent (the no-active-agent notification). + sends := ad.Sends() + if len(sends) == 0 { + t.Fatal("no reply sent to peer when agent is nil — silent drop is not allowed") + } + found := false + for _, s := range sends { + if strings.Contains(s.Text, "no active agent") || strings.Contains(s.Text, "agent") { + found = true + break + } + } + if !found { + t.Errorf("reply text does not mention the problem: %v", sends) + } +} + +// --------------------------------------------------------------------------- +// Task 3.2: TestBufferInbound_DropNotifiesEvictedPeer +// --------------------------------------------------------------------------- + +// TestBufferInbound_DropNotifiesEvictedPeer verifies that when the interactive +// buffer is full and a new message arrives, the evicted peer receives a +// notification and the buffer still holds exactly interactiveInboundBufferCap +// elements with the newest message at the tail. +func TestBufferInbound_DropNotifiesEvictedPeer(t *testing.T) { + t.Parallel() + svc, _ := newOrchestratorForTest(t) + ad := newStubAdapter("slack", "default") + svc.adapters[adapterKey("slack", "default")] = ad + + r := newBufferRouter(svc) + + ctx := context.Background() + + // Fill buffer to cap. All messages from peer "D1". + evictedPeer := bridge.PeerRef{Channel: "slack", Identity: "default", PeerID: "D1"} + for i := 0; i < interactiveInboundBufferCap; i++ { + r.BufferInbound(ctx, "S1", bridge.Inbound{ + Peer: evictedPeer, + Text: "old", + }) + } + if got := r.bufferedLen("S1"); got != interactiveInboundBufferCap { + t.Fatalf("buffer len = %d after filling, want cap %d", got, interactiveInboundBufferCap) + } + + // Push one more — should evict the oldest and notify its peer. + newest := bridge.Inbound{ + Peer: bridge.PeerRef{Channel: "slack", Identity: "default", PeerID: "D2"}, + Text: "newest", + } + r.BufferInbound(ctx, "S1", newest) + + // Buffer must still be at cap. + if got := r.bufferedLen("S1"); got != interactiveInboundBufferCap { + t.Errorf("buffer len = %d after eviction, want %d", got, interactiveInboundBufferCap) + } + + // The evicted peer (D1) must have received a notification. The notice is + // sent on a detached goroutine (BufferInbound runs on the SHARED inbound + // loop and must not block on platform I/O), so poll for it. + var sends []bridge.Outbound + for deadline := time.Now().Add(2 * time.Second); time.Now().Before(deadline); { + if sends = ad.Sends(); len(sends) > 0 { + break + } + time.Sleep(10 * time.Millisecond) + } + if len(sends) == 0 { + t.Fatal("no notification sent to evicted peer — silent drop is not allowed") + } + found := false + for _, s := range sends { + if strings.Contains(s.Text, "lost") || strings.Contains(s.Text, "dropped") || + strings.Contains(s.Text, "buffered") { + found = true + break + } + } + if !found { + t.Errorf("eviction notification text unexpected: %v", sends) + } + + // Newest message must be at the tail. + r.mu.Lock() + q := r.buffered["S1"] + tail := q[len(q)-1] + r.mu.Unlock() + if tail.Text != newest.Text { + t.Errorf("tail.Text = %q, want %q (newest should be at tail)", tail.Text, newest.Text) + } +} + +// --------------------------------------------------------------------------- +// Task 4.2: TestShutdown_WarnsOnQueuedMessages +// --------------------------------------------------------------------------- + +// TestShutdown_WarnsOnQueuedMessages verifies that close() emits one WARN log +// per dropped message (session ID + peer ID) plus a summary WARN, for both +// d.inbound items and d.overflow items. +// +// NOTE: this test modifies the global slog default and MUST NOT be run in +// parallel with other tests that also modify it. +func TestShutdown_WarnsOnQueuedMessages(t *testing.T) { + // Capture WARN-level log output. + var buf strings.Builder + prev := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelWarn}))) + t.Cleanup(func() { slog.SetDefault(prev) }) + + svc, _ := newOrchestratorForTest(t) + d := newBareDispatch(svc, "SHD") + + // Put 3 messages in d.inbound. + peerA := bridge.PeerRef{Channel: "slack", Identity: "default", PeerID: "peerA"} + peerB := bridge.PeerRef{Channel: "slack", Identity: "default", PeerID: "peerB"} + peerC := bridge.PeerRef{Channel: "slack", Identity: "default", PeerID: "peerC"} + d.inbound <- bridge.Inbound{Peer: peerA, Text: "msg1"} + d.inbound <- bridge.Inbound{Peer: peerB, Text: "msg2"} + d.inbound <- bridge.Inbound{Peer: peerC, Text: "msg3"} + + // Put 2 messages in d.overflow. + peerD := bridge.PeerRef{Channel: "slack", Identity: "default", PeerID: "peerD"} + peerE := bridge.PeerRef{Channel: "slack", Identity: "default", PeerID: "peerE"} + d.mu.Lock() + d.overflow = append(d.overflow, + bridge.Inbound{Peer: peerD, Text: "ov1"}, + bridge.Inbound{Peer: peerE, Text: "ov2"}, + ) + d.mu.Unlock() + + // close() should drain and log. + d.close() + + logged := buf.String() + // Per-message WARNs: each peer ID must appear. + for _, peer := range []string{"peerA", "peerB", "peerC", "peerD", "peerE"} { + if !strings.Contains(logged, peer) { + t.Errorf("WARN log missing peer %q; full log:\n%s", peer, logged) + } + } + // Session ID must appear. + if !strings.Contains(logged, "SHD") { + t.Errorf("WARN log missing session ID \"SHD\"; full log:\n%s", logged) + } + // Summary WARN must appear. + if !strings.Contains(logged, "dropped") && !strings.Contains(logged, "shutdown dropped") { + t.Errorf("no shutdown summary WARN found; full log:\n%s", logged) + } +} diff --git a/internal/bridge/service/dispatch_test.go b/internal/bridge/service/dispatch_test.go index 356ec8acda..86f459f630 100644 --- a/internal/bridge/service/dispatch_test.go +++ b/internal/bridge/service/dispatch_test.go @@ -2,11 +2,10 @@ package service import ( "errors" - "fmt" "strings" "testing" - "github.com/opencode-ai/opencode/internal/llm/agent" + agentpkg "github.com/opencode-ai/opencode/internal/llm/agent" "github.com/opencode-ai/opencode/internal/message" ) @@ -51,25 +50,35 @@ func TestAgentMessageTextSingleTextPart(t *testing.T) { } } -// TestRunFailureMessage_BusyDoesNotAdviseAbort: with the process-global -// session-run ledger, a session running under another agent instance (a flow -// step's own agent) now reports busy at this callsite. The generic advice -// ("POST /abort to release the busy lock") would cancel that live run — -// Cancel's cross-instance fallback reaches it — so ErrSessionBusy must get -// its own wait-and-resend text. +// TestRunFailureMessage_BusyDoesNotAdviseAbort was converted: ErrSessionBusy +// no longer reaches runFailureMessage — the retry loop in handleInbound handles +// it first. This test now verifies that: +// +// 1. runFailureMessage still works for non-busy errors (regression guard). +// 2. When given ErrSessionBusy directly (which can happen in edge cases), +// the generic message does NOT contain "resend" or abort-dangerous advice. +// (The prior version had a special "wait and resend" arm; removing it +// ensures the generic recovery hint is returned instead.) +// +// The full retry behavioral test lives in dispatch_busy_retry_test.go as +// TestHandleInbound_BusyRetryPreservesContent. func TestRunFailureMessage_BusyDoesNotAdviseAbort(t *testing.T) { - msg := runFailureMessage(agent.ErrSessionBusy, "flow-x-step1") - if strings.Contains(msg, "/abort") || strings.Contains(msg, "/reset") { - t.Errorf("busy message must not advise abort/reset: %q", msg) + // Verify ErrSessionBusy now produces the generic error message. + // Previously it returned a special "resend" message; now it is the + // same as any other error (the retry loop prevents it from reaching here). + msg := runFailureMessage(agentpkg.ErrSessionBusy, "flow-x-step1") + if msg == "" { + t.Fatal("runFailureMessage returned empty string") } - if !strings.Contains(msg, "resend") { - t.Errorf("busy message should tell the reviewer to resend: %q", msg) + // The generic message MUST contain the recovery hint (not suppress it). + if !strings.Contains(msg, "/abort") || !strings.Contains(msg, "/reset") { + t.Errorf("generic runFailureMessage missing recovery hint: %q", msg) } - - // Wrapped errors must take the same branch. - wrapped := fmt.Errorf("starting run: %w", agent.ErrSessionBusy) - if runFailureMessage(wrapped, "s") != msg { - t.Error("wrapped ErrSessionBusy did not take the busy branch") + // The "resend" wording was the old ErrSessionBusy-specific text. It must + // NOT appear — after the retry fix, if ErrSessionBusy somehow reaches here + // the user gets the generic escape hatch, not a misleading "resend" prompt. + if strings.Contains(msg, "resend") { + t.Errorf("generic message should not say \"resend\" (old busy-specific text): %q", msg) } } diff --git a/internal/bridge/service/http_inbound.go b/internal/bridge/service/http_inbound.go index 7841e17c2d..c9088df944 100644 --- a/internal/bridge/service/http_inbound.go +++ b/internal/bridge/service/http_inbound.go @@ -55,6 +55,13 @@ func (s *Service) handleInbound(w http.ResponseWriter, r *http.Request) { // response is moot. return default: - writeAPIError(w, http.StatusTooManyRequests, "inbound dispatcher full; retry") + // Channel full. Return a machine-readable 429 with Retry-After + // so the orchestrator's retry policy is actionable. + w.Header().Set("Retry-After", "1") + writeJSON(w, http.StatusTooManyRequests, map[string]any{ + "error": "inbound dispatcher full", + "retryAfterSeconds": 1, + "dispatcherSaturated": true, + }) } } diff --git a/internal/bridge/service/http_inbound_test.go b/internal/bridge/service/http_inbound_test.go index 06f8c7e5f6..57179fffaf 100644 --- a/internal/bridge/service/http_inbound_test.go +++ b/internal/bridge/service/http_inbound_test.go @@ -128,8 +128,9 @@ func TestRouterInbound_MalformedJSON(t *testing.T) { } // TestRouterInbound_BackpressureReturns429 verifies that when the shared -// inboundCh is full the handler responds with 429. Phase A.4 scenario -// "backpressure → 429". +// inboundCh is full the handler responds with 429, Retry-After: 1 header, +// and machine-readable body (dispatcherSaturated: true). Phase A.4 scenario +// "backpressure → 429" + bridge-http-api spec enrichment. func TestRouterInbound_BackpressureReturns429(t *testing.T) { svc, _ := newOrchestratorForTest(t) // Replace the channel with a length-1 capacity so we can fill it @@ -160,6 +161,21 @@ func TestRouterInbound_BackpressureReturns429(t *testing.T) { if resp.StatusCode != http.StatusTooManyRequests { t.Fatalf("status %d, want 429", resp.StatusCode) } + // Verify Retry-After header (task 5.1) + if got := resp.Header.Get("Retry-After"); got != "1" { + t.Errorf("Retry-After = %q, want \"1\"", got) + } + // Verify machine-readable body (task 5.1) + var respBody map[string]any + if err := json.NewDecoder(resp.Body).Decode(&respBody); err != nil { + t.Fatalf("decode response body: %v", err) + } + if sat, ok := respBody["dispatcherSaturated"]; !ok || sat != true { + t.Errorf("dispatcherSaturated = %v ok=%v, want true", sat, ok) + } + if errMsg, ok := respBody["error"]; !ok || errMsg == "" { + t.Errorf("error field missing or empty: %v", respBody) + } } // TestRouterInbound_AuthIsHandledByAPIMiddleware documents that the diff --git a/internal/bridge/service/inbound.go b/internal/bridge/service/inbound.go index f2eb86c254..feef2d44d0 100644 --- a/internal/bridge/service/inbound.go +++ b/internal/bridge/service/inbound.go @@ -131,7 +131,7 @@ func (s *Service) dispatchInbound(ctx context.Context, in bridge.Inbound) { // daemon agents and non-flow bridge chat. if s.questionRouter != nil && s.app != nil && s.app.Permissions != nil && s.app.Permissions.IsInteractiveSession(binding.SessionID) { - s.questionRouter.BufferInbound(binding.SessionID, in) + s.questionRouter.BufferInbound(ctx, binding.SessionID, in) logging.Info("bridge: buffered inbound for interactive session (no pending question)", "session", binding.SessionID, "peer", in.Peer.PeerID) return @@ -148,9 +148,7 @@ func (s *Service) dispatchInbound(ctx context.Context, in bridge.Inbound) { in.Text = PrependAttributionIfMultiPeer(in.Peer, in.Text, peerCount) disp := s.dispatcherFor(binding.SessionID) - if err := disp.pushInbound(ctx, in); err != nil { - logging.Warn("bridge: pushInbound", "session", binding.SessionID, "err", err) - } + disp.pushInbound(in) } // resolveBinding returns the binding for the inbound's peer, creating a diff --git a/internal/bridge/service/interactive_buffer_test.go b/internal/bridge/service/interactive_buffer_test.go index 44ab7afb01..bd6dbb6ff6 100644 --- a/internal/bridge/service/interactive_buffer_test.go +++ b/internal/bridge/service/interactive_buffer_test.go @@ -39,7 +39,7 @@ func TestBufferInbound_FIFOAndDropOldest(t *testing.T) { r := newBufferRouter(nil) for i := 0; i < interactiveInboundBufferCap+2; i++ { - r.BufferInbound("S1", bridge.Inbound{Text: strconv.Itoa(i)}) + r.BufferInbound(context.Background(), "S1", bridge.Inbound{Text: strconv.Itoa(i)}) } if got := r.bufferedLen("S1"); got != interactiveInboundBufferCap { t.Fatalf("buffer len = %d, want cap %d", got, interactiveInboundBufferCap) @@ -59,7 +59,7 @@ func TestClearSession_DropsPendingAndBuffered(t *testing.T) { t.Parallel() r := newBufferRouter(nil) r.pending["S1"] = &pendingQuestion{requestID: "req-1"} - r.BufferInbound("S1", bridge.Inbound{Text: "hi"}) + r.BufferInbound(context.Background(), "S1", bridge.Inbound{Text: "hi"}) r.ClearSession("S1") @@ -147,7 +147,7 @@ func TestHandleNewRequest_DrainsBufferedIntoReply(t *testing.T) { } // A reviewer message arrived while no question was pending. - r.BufferInbound("S1", bridge.Inbound{ + r.BufferInbound(context.Background(), "S1", bridge.Inbound{ Peer: bridge.PeerRef{Channel: "slack", Identity: "default", PeerID: "D1"}, Text: "Approve", }) diff --git a/internal/bridge/service/question.go b/internal/bridge/service/question.go index 733fe0462e..acbbc5414d 100644 --- a/internal/bridge/service/question.go +++ b/internal/bridge/service/question.go @@ -468,17 +468,51 @@ func formatAnswerAck(answers [][]string) string { // keeps every reply inside the flow agent's turn. // // FIFO with a drop-oldest cap so a reviewer firing many messages into -// the between-questions gap can't grow memory unbounded. -func (r *QuestionRouter) BufferInbound(sessionID string, in bridge.Inbound) { +// the between-questions gap can't grow memory unbounded. When eviction +// occurs, the evicted peer is notified so their message is not silently +// lost. +func (r *QuestionRouter) BufferInbound(ctx context.Context, sessionID string, in bridge.Inbound) { r.mu.Lock() - defer r.mu.Unlock() q := r.buffered[sessionID] + var evicted *bridge.Inbound if len(q) >= interactiveInboundBufferCap { logging.Warn("bridge: interactive inbound buffer full — dropping oldest", "session", sessionID, "cap", interactiveInboundBufferCap) + e := q[0] + evicted = &e q = q[1:] } r.buffered[sessionID] = append(q, in) + r.mu.Unlock() + + if evicted != nil && r.svc != nil { + // Fire-and-forget: BufferInbound runs on the SHARED + // orchestrator-inbound-dispatch goroutine (Service.runInboundLoop → + // dispatchInbound), so a synchronous platform send here stalls inbound + // dispatch for every session and identity for the duration of the call. + // That is worst for the orchestrator-mediated external channel, whose + // Send is an HTTP round-trip back to the orchestrator (10 s timeout). + // Mirrors emitToolUpdate's fire-and-forget posture in dispatch.go. + peer := evicted.Peer + // WithoutCancel keeps the caller's values (session scope, trace ids) + // while detaching the lifetime, so the send is not cancelled the moment + // dispatchInbound returns. Deliberately not r.svc.ctx: that is only set + // in Service.Start, and a nil ctx here would panic inside replyToPeer. + sendCtx := context.WithoutCancel(ctx) + go func() { + defer func() { + if rec := recover(); rec != nil { + logging.Warn("bridge: buffer-eviction notice panic", + "session", sessionID, "panic", rec) + } + }() + r.svc.replyToPeer(sendCtx, peer, + "bridge: your earlier message was lost because too many messages were buffered "+ + "while the interactive step had no pending question. "+ + "Please resend it once the current step completes.", + false, sessionID) + }() + } } // popBufferedLocked removes and returns the oldest buffered inbound for diff --git a/internal/bridge/slack/adapter.go b/internal/bridge/slack/adapter.go index 56c0a4e2f2..96746041a9 100644 --- a/internal/bridge/slack/adapter.go +++ b/internal/bridge/slack/adapter.go @@ -987,3 +987,47 @@ func truncateRunes(s string, maxRunes int) string { } return s } + +// Compile-time assertion: Adapter implements bridge.QueuedAcknowledger. +var _ bridge.QueuedAcknowledger = (*Adapter)(nil) + +// SendQueuedAck posts a queued-acknowledgement message to the peer and returns +// a token encoding the channel ID and message ts for subsequent in-place edits. +// Token format: channelID + "\x00" + ts (null-separated; neither field contains null bytes). +func (a *Adapter) SendQueuedAck(ctx context.Context, peer bridge.PeerRef, position int) (bridge.QueueAckToken, error) { + parsed := ParsePeerID(peer.PeerID) + if parsed.ChannelID == "" { + return "", ErrInvalidPeerID + } + opts := []slackgo.MsgOption{slackgo.MsgOptionText(bridge.QueueAckText(position), false)} + if parsed.ThreadTS != "" { + opts = append(opts, slackgo.MsgOptionTS(parsed.ThreadTS)) + } + _, ts, err := a.api.PostMessageContext(ctx, parsed.ChannelID, opts...) + if err != nil { + return "", fmt.Errorf("slack: SendQueuedAck: %w", err) + } + return parsed.ChannelID + "\x00" + ts, nil +} + +// UpdateQueuedAck edits the queued-ack message identified by token in-place. +// Token must be the value returned by SendQueuedAck (channelID + "\x00" + ts). +// Pass position == 0 to resolve the ack ("▶ Processing…"). +func (a *Adapter) UpdateQueuedAck(ctx context.Context, _ bridge.PeerRef, token bridge.QueueAckToken, position int) error { + parts := strings.SplitN(token, "\x00", 2) + if len(parts) != 2 || parts[0] == "" || parts[1] == "" { + return fmt.Errorf("slack: UpdateQueuedAck: invalid token %q", token) + } + channelID, ts := parts[0], parts[1] + var text string + if position == 0 { + text = bridge.ResolvedAckText + } else { + text = bridge.QueueAckText(position) + } + _, _, _, err := a.api.UpdateMessageContext(ctx, channelID, ts, slackgo.MsgOptionText(text, false)) + if err != nil { + return fmt.Errorf("slack: UpdateQueuedAck: %w", err) + } + return nil +} diff --git a/internal/bridge/slack/adapter_test.go b/internal/bridge/slack/adapter_test.go index 0170b4121b..e5d6cdcc9b 100644 --- a/internal/bridge/slack/adapter_test.go +++ b/internal/bridge/slack/adapter_test.go @@ -28,6 +28,7 @@ type mockSlackServer struct { mu sync.Mutex authTest int posts []postCall + updates []updateCall uploads []uploadCall opens []string files map[string]string // file ID → body @@ -46,6 +47,12 @@ type uploadCall struct { ThreadTS string } +type updateCall struct { + Channel string + TS string + Text string +} + func newMockServer(t *testing.T) *mockSlackServer { m := &mockSlackServer{t: t, files: map[string]string{}} mux := http.NewServeMux() @@ -95,6 +102,22 @@ func (m *mockSlackServer) handleAPI(w http.ResponseWriter, r *http.Request) { "user_id": "UBOT", "team_id": "T1", }) + case "chat.update": + _ = r.ParseForm() + call := updateCall{ + Channel: r.FormValue("channel"), + TS: r.FormValue("ts"), + Text: r.FormValue("text"), + } + m.mu.Lock() + m.updates = append(m.updates, call) + m.mu.Unlock() + m.respond(w, map[string]any{ + "ok": true, + "channel": call.Channel, + "ts": call.TS, + "text": call.Text, + }) case "chat.postMessage": _ = r.ParseForm() call := postCall{ diff --git a/internal/bridge/slack/queue_ack_test.go b/internal/bridge/slack/queue_ack_test.go new file mode 100644 index 0000000000..22b6d9b95a --- /dev/null +++ b/internal/bridge/slack/queue_ack_test.go @@ -0,0 +1,93 @@ +package slack + +import ( + "context" + "strings" + "testing" + + "github.com/opencode-ai/opencode/internal/bridge" +) + +// TestSendQueuedAck_Slack verifies SendQueuedAck calls PostMessageContext and +// returns a token encoding the channel ID and ts. +func TestSendQueuedAck_Slack(t *testing.T) { + t.Parallel() + a, mock, _ := newAdapter(t, Identity{ID: "default", BotToken: "xoxb-test", AppToken: "xapp-test"}) + + peer := bridge.PeerRef{Channel: "slack", Identity: "default", PeerID: "D0123TEST"} + tok, err := a.SendQueuedAck(context.Background(), peer, 1) + if err != nil { + t.Fatalf("SendQueuedAck: %v", err) + } + if tok == "" { + t.Fatal("SendQueuedAck returned empty token") + } + + // Token must encode channelID and ts separated by null byte. + parts := strings.SplitN(tok, "\x00", 2) + if len(parts) != 2 { + t.Fatalf("token format wrong, got %q (want channelID\\x00ts)", tok) + } + if parts[0] != "D0123TEST" { + t.Errorf("token channel = %q, want %q", parts[0], "D0123TEST") + } + if parts[1] == "" { + t.Error("token ts is empty") + } + + mock.mu.Lock() + posts := mock.posts + mock.mu.Unlock() + if len(posts) == 0 { + t.Fatal("PostMessageContext was not called") + } + if !strings.Contains(posts[len(posts)-1].Text, "⏳") { + t.Errorf("ack text missing ⏳ glyph: %q", posts[len(posts)-1].Text) + } +} + +// TestUpdateQueuedAck_Slack verifies UpdateQueuedAck calls UpdateMessageContext +// with the channel and ts from the token. +func TestUpdateQueuedAck_Slack(t *testing.T) { + t.Parallel() + a, mock, _ := newAdapter(t, Identity{ID: "default", BotToken: "xoxb-test", AppToken: "xapp-test"}) + + peer := bridge.PeerRef{Channel: "slack", Identity: "default", PeerID: "D0123TEST"} + tok, err := a.SendQueuedAck(context.Background(), peer, 1) + if err != nil { + t.Fatalf("SendQueuedAck: %v", err) + } + + // In-place update with position. + if err := a.UpdateQueuedAck(context.Background(), peer, tok, 1); err != nil { + t.Fatalf("UpdateQueuedAck position=1: %v", err) + } + // Resolve. + if err := a.UpdateQueuedAck(context.Background(), peer, tok, 0); err != nil { + t.Fatalf("UpdateQueuedAck position=0 (resolve): %v", err) + } + + mock.mu.Lock() + updates := mock.updates + mock.mu.Unlock() + + if len(updates) < 2 { + t.Fatalf("chat.update called %d times, want ≥2", len(updates)) + } + last := updates[len(updates)-1] + if !strings.Contains(last.Text, "▶") { + t.Errorf("resolve update text missing ▶ glyph: %q", last.Text) + } +} + +// TestUpdateQueuedAck_Slack_InvalidToken rejects a malformed token. +func TestUpdateQueuedAck_Slack_InvalidToken(t *testing.T) { + t.Parallel() + a, _, _ := newAdapter(t, Identity{ID: "default", BotToken: "xoxb-test", AppToken: "xapp-test"}) + err := a.UpdateQueuedAck(context.Background(), + bridge.PeerRef{Channel: "slack", Identity: "default", PeerID: "D0123TEST"}, + "no-null-byte-token", 1) + if err == nil { + t.Error("expected error for malformed token") + } +} diff --git a/internal/bridge/telegram/adapter.go b/internal/bridge/telegram/adapter.go index 718be3c9aa..b49b96c9e0 100644 --- a/internal/bridge/telegram/adapter.go +++ b/internal/bridge/telegram/adapter.go @@ -1118,3 +1118,55 @@ func redactToken(s, token string) string { } return strings.ReplaceAll(s, token, "") } + +// Compile-time assertion: Adapter implements bridge.QueuedAcknowledger. +var _ bridge.QueuedAcknowledger = (*Adapter)(nil) + +// SendQueuedAck sends a queued-acknowledgement message to the peer and returns +// the message ID (string-encoded) as the token for subsequent in-place edits. +// position is 1-based (1 = "you're next"). +func (a *Adapter) SendQueuedAck(ctx context.Context, peer bridge.PeerRef, position int) (bridge.QueueAckToken, error) { + chatID, err := ParsePeerID(peer.PeerID) + if err != nil { + return "", err + } + msg, err := a.bot.SendMessage(ctx, &tgbot.SendMessageParams{ + ChatID: chatID, + Text: bridge.QueueAckText(position), + }) + if err != nil { + return "", fmt.Errorf("telegram: SendQueuedAck: %w", err) + } + if msg == nil { + return "", fmt.Errorf("telegram: SendQueuedAck: nil response") + } + return strconv.Itoa(msg.ID), nil +} + +// UpdateQueuedAck edits the queued-ack message identified by token in-place. +// Pass position == 0 to resolve the ack (run started → "▶ Processing…"). +func (a *Adapter) UpdateQueuedAck(ctx context.Context, peer bridge.PeerRef, token bridge.QueueAckToken, position int) error { + chatID, err := ParsePeerID(peer.PeerID) + if err != nil { + return err + } + msgID, err := strconv.Atoi(token) + if err != nil { + return fmt.Errorf("telegram: UpdateQueuedAck: invalid token %q: %w", token, err) + } + var text string + if position == 0 { + text = bridge.ResolvedAckText + } else { + text = bridge.QueueAckText(position) + } + _, err = a.bot.EditMessageText(ctx, &tgbot.EditMessageTextParams{ + ChatID: chatID, + MessageID: msgID, + Text: text, + }) + if err != nil { + return fmt.Errorf("telegram: UpdateQueuedAck: %w", err) + } + return nil +} diff --git a/internal/bridge/telegram/queue_ack_test.go b/internal/bridge/telegram/queue_ack_test.go new file mode 100644 index 0000000000..895935c15c --- /dev/null +++ b/internal/bridge/telegram/queue_ack_test.go @@ -0,0 +1,117 @@ +package telegram + +import ( + "context" + "strings" + "testing" + + "github.com/opencode-ai/opencode/internal/bridge" +) + +// TestSendQueuedAck verifies that SendQueuedAck calls sendMessage and returns +// the message ID (string-encoded) as the token. +func TestSendQueuedAck(t *testing.T) { + t.Parallel() + a, mock, _ := newAdapter(t, Identity{ID: "tg", Token: "tok"}) + + peer := bridge.PeerRef{Channel: "telegram", Identity: "tg", PeerID: "12345"} + tok, err := a.SendQueuedAck(context.Background(), peer, 1) + if err != nil { + t.Fatalf("SendQueuedAck: %v", err) + } + if tok == "" { + t.Fatal("SendQueuedAck returned empty token") + } + + // The mock returns message ID 1 for sendMessage. + if tok != "1" { + t.Errorf("token = %q, want \"1\"", tok) + } + + mock.mu.Lock() + sends := mock.sendMsg + mock.mu.Unlock() + if len(sends) == 0 { + t.Fatal("sendMessage was not called") + } + if !strings.Contains(sends[len(sends)-1].Text, "⏳") { + t.Errorf("ack text missing ⏳ glyph: %q", sends[len(sends)-1].Text) + } +} + +// TestUpdateQueuedAck_InPlace verifies that UpdateQueuedAck calls editMessageText +// with the correct message ID and position text. +func TestUpdateQueuedAck_InPlace(t *testing.T) { + t.Parallel() + a, mock, _ := newAdapter(t, Identity{ID: "tg", Token: "tok"}) + + peer := bridge.PeerRef{Channel: "telegram", Identity: "tg", PeerID: "12345"} + // Send first, then update. + tok, err := a.SendQueuedAck(context.Background(), peer, 1) + if err != nil { + t.Fatalf("SendQueuedAck: %v", err) + } + + if err := a.UpdateQueuedAck(context.Background(), peer, tok, 1); err != nil { + t.Fatalf("UpdateQueuedAck position=1: %v", err) + } + if err := a.UpdateQueuedAck(context.Background(), peer, tok, 0); err != nil { + t.Fatalf("UpdateQueuedAck position=0 (resolve): %v", err) + } + + mock.mu.Lock() + edits := mock.editMessageText + mock.mu.Unlock() + + if len(edits) < 2 { + t.Fatalf("editMessageText called %d times, want ≥2", len(edits)) + } + // Last edit must be the resolve text. + if !strings.Contains(edits[len(edits)-1].Text, "▶") { + t.Errorf("resolve edit text missing ▶ glyph: %q", edits[len(edits)-1].Text) + } + // Message ID must round-trip through the token. + if edits[len(edits)-1].MessageID != tok { + t.Errorf("message_id in edit = %q, want token %q", edits[len(edits)-1].MessageID, tok) + } +} + +// TestSendQueuedAck_InvalidPeer rejects a non-numeric peer ID. +func TestSendQueuedAck_InvalidPeer(t *testing.T) { + t.Parallel() + a, _, _ := newAdapter(t, Identity{ID: "tg", Token: "tok"}) + _, err := a.SendQueuedAck(context.Background(), + bridge.PeerRef{Channel: "telegram", Identity: "tg", PeerID: "@notanumber"}, + 1) + if err == nil { + t.Error("expected error for invalid peer ID") + } +} + +// TestUpdateQueuedAck_InvalidToken rejects a non-numeric token. +func TestUpdateQueuedAck_InvalidToken(t *testing.T) { + t.Parallel() + a, _, _ := newAdapter(t, Identity{ID: "tg", Token: "tok"}) + err := a.UpdateQueuedAck(context.Background(), + bridge.PeerRef{Channel: "telegram", Identity: "tg", PeerID: "12345"}, + "notanumber", 1) + if err == nil { + t.Error("expected error for invalid token") + } +} + +// TestQueuedAckPosition verifies position==2 text differs from position==1. +func TestQueuedAckPosition(t *testing.T) { + t.Parallel() + text1 := bridge.QueueAckText(1) + text2 := bridge.QueueAckText(2) + if text1 == text2 { + t.Errorf("position 1 and 2 produced identical text: %q", text1) + } + if !strings.Contains(text1, "⏳") || !strings.Contains(text2, "⏳") { + t.Errorf("ack text missing ⏳ glyph: %q / %q", text1, text2) + } + if !strings.Contains(bridge.ResolvedAckText, "▶") { + t.Errorf("resolved text missing ▶ glyph: %q", bridge.ResolvedAckText) + } +} diff --git a/internal/config/config_router_test.go b/internal/config/config_router_test.go new file mode 100644 index 0000000000..035caefca1 --- /dev/null +++ b/internal/config/config_router_test.go @@ -0,0 +1,73 @@ +package config + +import ( + "os" + "path/filepath" + "testing" + + "github.com/spf13/viper" +) + +// TestConfig_RouterQueueAcksViperRoundTrip locks in that +// `router.queueAcknowledgementsEnabled` survives the real loader path +// (viper.ReadInConfig + viper.Unmarshal). Viper case-folds keys to lowercase +// during JSON ingestion; a camelCase field whose name maps to something Viper +// case-folds unexpectedly would be silently dropped. This test catches that +// before the field ships — a pure json.Unmarshal test would pass while the +// real config loader mangles it in production. +func TestConfig_RouterQueueAcksViperRoundTrip(t *testing.T) { + dir := t.TempDir() + body := `{"router": {"queueAcknowledgementsEnabled": true}}` + if err := os.WriteFile(filepath.Join(dir, ".opencode.json"), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + + v := viper.New() + v.SetConfigName(".opencode") + v.SetConfigType("json") + v.AddConfigPath(dir) + if err := v.ReadInConfig(); err != nil { + t.Fatalf("read: %v", err) + } + var cfg Config + if err := v.Unmarshal(&cfg); err != nil { + t.Fatalf("unmarshal: %v", err) + } + + if cfg.Router == nil { + t.Fatal("router config was dropped by the loader") + } + if !cfg.Router.QueueAcknowledgementsEnabled { + t.Errorf("QueueAcknowledgementsEnabled = false after round-trip, want true") + } +} + +// TestConfig_RouterQueueAcksDefaultFalse verifies that the field defaults to +// false when omitted from .opencode.json (no-op for the absent key, but Viper +// must not set it to true on any implicit default path). +func TestConfig_RouterQueueAcksDefaultFalse(t *testing.T) { + dir := t.TempDir() + body := `{"router": {"toolUpdatesEnabled": true}}` + if err := os.WriteFile(filepath.Join(dir, ".opencode.json"), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + + v := viper.New() + v.SetConfigName(".opencode") + v.SetConfigType("json") + v.AddConfigPath(dir) + if err := v.ReadInConfig(); err != nil { + t.Fatalf("read: %v", err) + } + var cfg Config + if err := v.Unmarshal(&cfg); err != nil { + t.Fatalf("unmarshal: %v", err) + } + + if cfg.Router == nil { + t.Fatal("router config was dropped by the loader") + } + if cfg.Router.QueueAcknowledgementsEnabled { + t.Errorf("QueueAcknowledgementsEnabled = true when omitted, want false (default)") + } +} diff --git a/internal/cron/scheduler.go b/internal/cron/scheduler.go index 769c2132c1..a56e578e6f 100644 --- a/internal/cron/scheduler.go +++ b/internal/cron/scheduler.go @@ -491,6 +491,23 @@ func (s *Scheduler) fireJob(ctx context.Context, job CronJob) { } logging.Info("Cron job firing", "id", job.ID, "schedule", job.Schedule, "title", job.TaskTitle) + // Keep the session's "is anybody watching?" verdict current for the + // question tool, using the same predicate as the permission gate + // above: the TUI's selected session or a bridge-bound one can answer a + // question; anything else (an auto-approved session in a headless + // deploy) cannot, and a blocking `question` there would wedge the + // session's agent lock and every later run of this job. Re-evaluated on + // every fire so the session becomes attended again the moment the TUI + // selects it or a reviewer binds chat to it. Subagent sessions — which + // is what a cron job actually runs in — inherit the mark through the + // permission session-link chain. + if s.permissions != nil { + if job.SessionID == s.activeSessionID() || s.hasPermissionResolver(ctx, job.SessionID) { + s.permissions.RemoveUnattendedSession(job.SessionID) + } else { + s.permissions.MarkUnattendedSession(job.SessionID) + } + } // The job is executing again — re-arm the one-shot deferral log so a // future unwatched stretch is reported anew. s.deferLoggedJobs.Delete(job.ID) diff --git a/internal/flow/service.go b/internal/flow/service.go index e519e422bb..a34f281649 100644 --- a/internal/flow/service.go +++ b/internal/flow/service.go @@ -529,6 +529,15 @@ func (s *service) runStep( } s.permissions.AutoApproveSession(sess.ID) + // A flow step runs unattended: no TUI dialog and — unless it's an + // `interactive: true` step — no chat binding either, so the question + // tool answers itself rather than blocking on a prompt nobody sees. + // Marked for interactive steps too: their own session additionally + // carries the interactive marker (below), which the question tool + // checks first, while subagents they spawn are NOT bridge-bound and + // must keep auto-answering (they inherit this mark through the + // permission session-link chain). + s.permissions.MarkUnattendedSession(sess.ID) status := FlowStatusRunning if prevState != nil && postpone { diff --git a/internal/flow/service_fresh_test.go b/internal/flow/service_fresh_test.go index fae9808158..5ebc9387ad 100644 --- a/internal/flow/service_fresh_test.go +++ b/internal/flow/service_fresh_test.go @@ -164,6 +164,8 @@ type stubPermissions struct { func (p *stubPermissions) AutoApproveSession(_ string) {} +func (p *stubPermissions) MarkUnattendedSession(_ string) {} + // stubAgent returns a response event immediately. If responses is non-empty, // successive Run calls return the scripted events in order; otherwise a default // "done" text response is returned. Prompts received are captured into diff --git a/internal/flow/struct_output_retry_test.go b/internal/flow/struct_output_retry_test.go index 1ef45d97a4..e1f06600b4 100644 --- a/internal/flow/struct_output_retry_test.go +++ b/internal/flow/struct_output_retry_test.go @@ -70,10 +70,15 @@ type interactivePermissions struct { mu sync.Mutex interactive map[string]bool autoApprove map[string]bool + unattended map[string]bool } func newInteractivePermissions() *interactivePermissions { - return &interactivePermissions{interactive: map[string]bool{}, autoApprove: map[string]bool{}} + return &interactivePermissions{ + interactive: map[string]bool{}, + autoApprove: map[string]bool{}, + unattended: map[string]bool{}, + } } func (p *interactivePermissions) AutoApproveSession(sessionID string) { @@ -106,6 +111,18 @@ func (p *interactivePermissions) IsAutoApproveSession(sessionID string) bool { return p.autoApprove[sessionID] } +func (p *interactivePermissions) MarkUnattendedSession(sessionID string) { + p.mu.Lock() + defer p.mu.Unlock() + p.unattended[sessionID] = true +} + +func (p *interactivePermissions) IsUnattendedSession(sessionID string) bool { + p.mu.Lock() + defer p.mu.Unlock() + return p.unattended[sessionID] +} + // stubMessages is the slice of message.Service the failure-diagnostics path // uses: ListLatest, to recover the agent's last words for the step error. type stubMessages struct { diff --git a/internal/llm/tools/question.go b/internal/llm/tools/question.go index 53774d9689..609a0f822f 100644 --- a/internal/llm/tools/question.go +++ b/internal/llm/tools/question.go @@ -104,20 +104,28 @@ func (q *questionTool) Run(ctx context.Context, call ToolCall) (ToolResponse, er return NewTextErrorResponse("No active session"), nil } - // When auto-approve is active, auto-select the first option for each - // question instead of blocking on user input. The tool description - // instructs the LLM to put the recommended option first. + // Auto-answer (first option — the tool description instructs the LLM + // to put the recommended one first) ONLY when nobody can answer: + // an unattended session, i.e. a headless `opencode -p` run or a flow + // step (permission.Service.MarkUnattendedSession, inherited by + // task-tool subagents through the session link chain). // - // EXCEPTION: when the session is also marked as interactive (i.e. - // it's a `interactive: true` flow step bound to a human reviewer - // via the chat bridge), DO NOT auto-approve. The whole point of - // the interactive step is that the human picks the answer — the - // auto-approve would silently steal that turn and the bridge - // roundtrip would never happen. Defer to q.service.Ask which - // publishes the request to the broker so the bridge can route it - // to Slack/Telegram/Mattermost and wait for the human reply. + // Auto-approve alone is NOT enough. An attended surface — the TUI + // with auto-approve on, a chat-bridge session, an API client — must + // still see the real prompt: auto-approve is about tool permissions, + // not about answering the user's questions for them. + // + // EXCEPTION: when the session is marked as interactive (i.e. it's an + // `interactive: true` flow step bound to a human reviewer via the + // chat bridge), DO NOT auto-answer. The whole point of the + // interactive step is that the human picks the answer — auto-answering + // would silently steal that turn and the bridge roundtrip would never + // happen. Defer to q.service.Ask which publishes the request to the + // broker so the surface (TUI dialog, bridge fan-out to + // Slack/Telegram/Mattermost, API SSE) can collect the human reply. if q.permissions != nil && q.permissions.IsAutoApproveSession(sessionID) && + q.permissions.IsUnattendedSession(sessionID) && !q.permissions.IsInteractiveSession(sessionID) { answers := make([][]string, len(params.Questions)) for i, prompt := range params.Questions { diff --git a/internal/llm/tools/question_test.go b/internal/llm/tools/question_test.go index 8013958431..ff0b3e192c 100644 --- a/internal/llm/tools/question_test.go +++ b/internal/llm/tools/question_test.go @@ -196,6 +196,8 @@ func TestQuestionToolRunNoOptionsNoCustom(t *testing.T) { type mockPermissionService struct { autoApproved map[string]bool + unattended map[string]bool + interactive map[string]bool } func (m *mockPermissionService) Grant(_ permission.PermissionRequest) {} @@ -213,20 +215,42 @@ func (m *mockPermissionService) RemoveAutoApproveSession(id string) { func (m *mockPermissionService) IsAutoApproveSession(id string) bool { return m.autoApproved[id] } -func (m *mockPermissionService) LinkSession(_, _ string) {} -func (m *mockPermissionService) MarkInteractiveSession(_ string) {} -func (m *mockPermissionService) RemoveInteractiveSession(_ string) {} -func (m *mockPermissionService) IsInteractiveSession(_ string) bool { return false } +func (m *mockPermissionService) LinkSession(_, _ string) {} +func (m *mockPermissionService) MarkInteractiveSession(id string) { + if m.interactive == nil { + m.interactive = map[string]bool{} + } + m.interactive[id] = true +} +func (m *mockPermissionService) RemoveInteractiveSession(id string) { + delete(m.interactive, id) +} +func (m *mockPermissionService) IsInteractiveSession(id string) bool { return m.interactive[id] } +func (m *mockPermissionService) MarkUnattendedSession(id string) { + if m.unattended == nil { + m.unattended = map[string]bool{} + } + m.unattended[id] = true +} +func (m *mockPermissionService) RemoveUnattendedSession(id string) { + delete(m.unattended, id) +} +func (m *mockPermissionService) IsUnattendedSession(id string) bool { return m.unattended[id] } func (m *mockPermissionService) Subscribe(_ context.Context) <-chan pubsub.Event[permission.PermissionRequest] { return nil } +// An unattended session (headless `-p` run, flow step) has no surface that +// could answer, so the tool picks the first (recommended) option itself. func TestQuestionToolAutoApprove(t *testing.T) { svc := &mockQuestionService{askFn: func(_ context.Context, _ string, _ []question.Prompt) ([][]string, error) { - t.Fatal("Ask should not be called when auto-approve is active") + t.Fatal("Ask should not be called for an unattended auto-approved session") return nil, nil }} - perms := &mockPermissionService{autoApproved: map[string]bool{"test-session": true}} + perms := &mockPermissionService{ + autoApproved: map[string]bool{"test-session": true}, + unattended: map[string]bool{"test-session": true}, + } tool := NewQuestionTool(svc, perms) input, _ := json.Marshal(questionParams{ @@ -253,3 +277,64 @@ func TestQuestionToolAutoApprove(t *testing.T) { t.Errorf("expected auto-selected first option, got: %s", resp.Content) } } + +// autoApproveAskProbe drives the two "auto-approve alone must NOT auto-answer" +// cases: an attended surface (TUI with auto-approve on, chat bridge, API) and +// an interactive flow step. Both must reach Ask so a human picks the answer. +func autoApproveAskProbe(t *testing.T, perms *mockPermissionService) { + t.Helper() + asked := false + svc := &mockQuestionService{ + askFn: func(_ context.Context, _ string, _ []question.Prompt) ([][]string, error) { + asked = true + return [][]string{{"Second"}}, nil + }, + } + tool := NewQuestionTool(svc, perms) + + input, _ := json.Marshal(questionParams{ + Questions: []question.Prompt{ + { + Question: "Pick one", + Options: []question.Option{ + {Label: "First (Recommended)", Description: "The recommended option"}, + {Label: "Second", Description: "Another option"}, + }, + }, + }, + }) + + ctx := context.WithValue(context.Background(), SessionIDContextKey, "test-session") + resp, err := tool.Run(ctx, ToolCall{ID: "1", Name: QuestionToolName, Input: string(input)}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if resp.IsError { + t.Fatalf("expected success, got error: %s", resp.Content) + } + if !asked { + t.Fatal("expected Ask to be called instead of auto-answering") + } + if !strings.Contains(resp.Content, "Second") { + t.Errorf("expected the user's answer, got: %s", resp.Content) + } +} + +// Auto-approve is about tool permissions, not about answering for the user: +// an attended session (TUI with auto-approve, bridge chat, API client) still +// gets the real prompt. +func TestQuestionToolAutoApproveAttendedStillAsks(t *testing.T) { + autoApproveAskProbe(t, &mockPermissionService{ + autoApproved: map[string]bool{"test-session": true}, + }) +} + +// An `interactive: true` flow step is unattended-marked (so its subagents keep +// auto-answering) but bound to a human reviewer — the step itself must ask. +func TestQuestionToolUnattendedInteractiveStillAsks(t *testing.T) { + autoApproveAskProbe(t, &mockPermissionService{ + autoApproved: map[string]bool{"test-session": true}, + unattended: map[string]bool{"test-session": true}, + interactive: map[string]bool{"test-session": true}, + }) +} diff --git a/internal/permission/mocks/permission_mock.go b/internal/permission/mocks/permission_mock.go index 103d59946e..9ab52b1b31 100644 --- a/internal/permission/mocks/permission_mock.go +++ b/internal/permission/mocks/permission_mock.go @@ -118,6 +118,20 @@ func (mr *MockServiceMockRecorder) IsInteractiveSession(sessionID any) *gomock.C return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsInteractiveSession", reflect.TypeOf((*MockService)(nil).IsInteractiveSession), sessionID) } +// IsUnattendedSession mocks base method. +func (m *MockService) IsUnattendedSession(sessionID string) bool { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "IsUnattendedSession", sessionID) + ret0, _ := ret[0].(bool) + return ret0 +} + +// IsUnattendedSession indicates an expected call of IsUnattendedSession. +func (mr *MockServiceMockRecorder) IsUnattendedSession(sessionID any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsUnattendedSession", reflect.TypeOf((*MockService)(nil).IsUnattendedSession), sessionID) +} + // LinkSession mocks base method. func (m *MockService) LinkSession(sessionID, parentSessionID string) { m.ctrl.T.Helper() @@ -142,6 +156,18 @@ func (mr *MockServiceMockRecorder) MarkInteractiveSession(sessionID any) *gomock return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "MarkInteractiveSession", reflect.TypeOf((*MockService)(nil).MarkInteractiveSession), sessionID) } +// MarkUnattendedSession mocks base method. +func (m *MockService) MarkUnattendedSession(sessionID string) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "MarkUnattendedSession", sessionID) +} + +// MarkUnattendedSession indicates an expected call of MarkUnattendedSession. +func (mr *MockServiceMockRecorder) MarkUnattendedSession(sessionID any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "MarkUnattendedSession", reflect.TypeOf((*MockService)(nil).MarkUnattendedSession), sessionID) +} + // RemoveAutoApproveSession mocks base method. func (m *MockService) RemoveAutoApproveSession(sessionID string) { m.ctrl.T.Helper() @@ -166,6 +192,18 @@ func (mr *MockServiceMockRecorder) RemoveInteractiveSession(sessionID any) *gomo return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RemoveInteractiveSession", reflect.TypeOf((*MockService)(nil).RemoveInteractiveSession), sessionID) } +// RemoveUnattendedSession mocks base method. +func (m *MockService) RemoveUnattendedSession(sessionID string) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "RemoveUnattendedSession", sessionID) +} + +// RemoveUnattendedSession indicates an expected call of RemoveUnattendedSession. +func (mr *MockServiceMockRecorder) RemoveUnattendedSession(sessionID any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RemoveUnattendedSession", reflect.TypeOf((*MockService)(nil).RemoveUnattendedSession), sessionID) +} + // Request mocks base method. func (m *MockService) Request(ctx context.Context, opts permission.CreatePermissionRequest) bool { m.ctrl.T.Helper() diff --git a/internal/permission/permission.go b/internal/permission/permission.go index 6051c39c0e..8afd993715 100644 --- a/internal/permission/permission.go +++ b/internal/permission/permission.go @@ -60,6 +60,29 @@ type Service interface { MarkInteractiveSession(sessionID string) RemoveInteractiveSession(sessionID string) IsInteractiveSession(sessionID string) bool + + // MarkUnattendedSession flags a session as running with no surface + // that can answer an agent's `question` call: a headless `opencode -p` + // run and flow steps. ONLY these sessions get the question tool's + // auto-answer short-circuit — every attended surface (TUI, chat + // bridge, API) shows the real prompt even when the session is + // auto-approved, so the human picks the answer instead of the agent + // silently taking the recommended option. + // + // Resolves through the LinkSession chain so task-tool subagents + // inherit it from their caller: a subagent spawned inside a flow step + // is just as unattended as the step itself. + // + // Interactive flow steps are marked unattended too — their own + // session carries the interactive marker, which the question tool + // checks first, while their subagents (not bridge-bound, so nobody + // could answer them) keep auto-answering. + MarkUnattendedSession(sessionID string) + // RemoveUnattendedSession clears the mark when a human attaches to + // the session after the fact (e.g. the bridge's `/session ` + // switch repoints a reviewer's binding at a former flow session). + RemoveUnattendedSession(sessionID string) + IsUnattendedSession(sessionID string) bool } type permissionService struct { @@ -69,6 +92,7 @@ type permissionService struct { pendingRequests sync.Map autoApproveSessions sync.Map interactiveSessions sync.Map + unattendedSessions sync.Map sessionParents sync.Map // child session ID -> parent session ID serializePermissions sync.Mutex } @@ -216,6 +240,21 @@ func (s *permissionService) IsInteractiveSession(sessionID string) bool { return ok } +func (s *permissionService) MarkUnattendedSession(sessionID string) { + s.unattendedSessions.Store(sessionID, true) +} + +func (s *permissionService) RemoveUnattendedSession(sessionID string) { + s.unattendedSessions.Delete(sessionID) +} + +func (s *permissionService) IsUnattendedSession(sessionID string) bool { + return s.walkSessionChain(sessionID, func(id string) bool { + _, ok := s.unattendedSessions.Load(id) + return ok + }) +} + func NewPermissionService() Service { return &permissionService{ Broker: pubsub.NewBroker[PermissionRequest](), diff --git a/internal/permission/permission_test.go b/internal/permission/permission_test.go index 3d5b108647..c0c449e58c 100644 --- a/internal/permission/permission_test.go +++ b/internal/permission/permission_test.go @@ -241,3 +241,31 @@ func TestPersistentGrantCoversLinkedSubagents(t *testing.T) { t.Fatal("expected child grant to not cover the parent session") } } + +// The unattended mark is what licenses the question tool to answer itself. +// It must resolve through the session-link chain (so a subagent spawned by a +// flow step is unattended too) and be clearable (a reviewer switching a chat +// binding onto a former flow session attends it). +func TestUnattendedSession(t *testing.T) { + svc := NewPermissionService() + + if svc.IsUnattendedSession("step") { + t.Fatal("expected a fresh session to be attended") + } + + svc.MarkUnattendedSession("step") + if !svc.IsUnattendedSession("step") { + t.Fatal("expected marked session to be unattended") + } + + // Subagent of the step (agent-tool.go links it to its caller). + svc.LinkSession("subagent", "step") + if !svc.IsUnattendedSession("subagent") { + t.Fatal("expected subagent session to inherit unattended from its caller") + } + + svc.RemoveUnattendedSession("step") + if svc.IsUnattendedSession("subagent") { + t.Fatal("expected clearing the parent to attend the subagent too") + } +} diff --git a/internal/tui/components/chat/editor.go b/internal/tui/components/chat/editor.go index 2fa5717cb4..a14c51c871 100644 --- a/internal/tui/components/chat/editor.go +++ b/internal/tui/components/chat/editor.go @@ -160,19 +160,37 @@ func (m *editorCmp) Init() tea.Cmd { } func (m *editorCmp) send() tea.Cmd { - if m.app.ActiveAgent().IsSessionBusy(m.session.ID) { - return util.ReportWarn("Agent is working, please wait...") - } - + // Always check for empty input first — an empty submit is a no-op + // regardless of queue or busy state (task 3.3). value := m.textarea.Value() - m.textarea.Reset() + if value == "" { + return nil + } attachments := m.attachments - m.attachments = nil - m.syncTextareaHeight() - if value == "" { + // FIFO routing (Decision 8, task 3.1): enqueue whenever the queue is + // non-empty OR the session is busy. Direct dispatch is only permitted + // when BOTH are false — queue empty AND session observably idle. Without + // the queue-non-empty branch, a submission arriving in the idle window + // between two drain deliveries would bypass already-queued messages. + if m.app.QueueLen(m.session.ID) > 0 || m.app.ActiveAgent().IsSessionBusy(m.session.ID) { + m.app.EnqueueMessage(m.session.ID, app.QueuedMessage{ + Text: value, + Attachments: attachments, + }) + m.textarea.Reset() + m.attachments = nil + // syncTextareaHeight MUST follow every mutation of m.attachments and + // never run from View (chat-editor-layout spec). + m.syncTextareaHeight() return nil } + + // Queue empty AND session idle: fall through to the direct dispatch path + // (today's behavior, unchanged). + m.textarea.Reset() + m.attachments = nil + m.syncTextareaHeight() return tea.Batch( util.CmdHandler(SendMsg{ Text: value, @@ -407,9 +425,21 @@ func (m *editorCmp) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } if key.Matches(msg, editorMaps.OpenEditor) { + // Queuing Ctrl+E / external $EDITOR sessions is a future decision; + // keep the busy-reject guard unchanged (task 6.1). The queue-length + // arm is what preserves FIFO: openEditor's result is delivered via + // SendMsg → chatPage.sendMessage, which dispatches directly and + // would jump ahead of already-queued messages. if m.app.ActiveAgent().IsSessionBusy(m.session.ID) { return m, util.ReportWarn("Agent is working, please wait...") } + if m.app.QueueLen(m.session.ID) > 0 { + // Distinct message: the session is idle here (e.g. the drain + // worker halted on an error), so "Agent is working" would be + // factually wrong and leave the user with no idea why ctrl+e + // is locked. + return m, util.ReportWarn("Messages are queued — wait for them to send, press ctrl+g to view or ctrl+x to discard") + } return m, m.openEditor() } if key.Matches(msg, DeleteKeyMaps.Escape) { diff --git a/internal/tui/components/chat/editor_busy_test.go b/internal/tui/components/chat/editor_busy_test.go new file mode 100644 index 0000000000..5f34c0ca0e --- /dev/null +++ b/internal/tui/components/chat/editor_busy_test.go @@ -0,0 +1,242 @@ +package chat + +import ( + "context" + "errors" + "testing" + + tea "charm.land/bubbletea/v2" + "github.com/opencode-ai/opencode/internal/app" + agentpkg "github.com/opencode-ai/opencode/internal/llm/agent" + "github.com/opencode-ai/opencode/internal/llm/models" + "github.com/opencode-ai/opencode/internal/llm/tools" + "github.com/opencode-ai/opencode/internal/message" + "github.com/opencode-ai/opencode/internal/pubsub" + "github.com/opencode-ai/opencode/internal/session" + + "github.com/opencode-ai/opencode/internal/config" +) + +// ---- fake agent for editor tests ------------------------------------------- + +type editorFakeAgent struct { + busy bool + runErr error + runChan chan agentpkg.AgentEvent // if nil, returns a closed channel +} + +func (a *editorFakeAgent) IsSessionBusy(_ string) bool { return a.busy } + +func (a *editorFakeAgent) Run(_ context.Context, _ string, _ string, _ int, _ ...message.Attachment) (<-chan agentpkg.AgentEvent, error) { + if a.runErr != nil { + return nil, a.runErr + } + if a.runChan != nil { + return a.runChan, nil + } + ch := make(chan agentpkg.AgentEvent) + close(ch) + return ch, nil +} + +// Satisfy the rest of the interface: +func (a *editorFakeAgent) Subscribe(_ context.Context) <-chan pubsub.Event[agentpkg.AgentEvent] { + ch := make(chan pubsub.Event[agentpkg.AgentEvent]) + close(ch) + return ch +} +func (a *editorFakeAgent) AgentID() config.AgentName { return "" } +func (a *editorFakeAgent) Model() models.Model { return models.Model{} } +func (a *editorFakeAgent) Tools() []tools.BaseTool { return nil } +func (a *editorFakeAgent) ResolvedTools() ([]tools.BaseTool, bool) { return nil, false } +func (a *editorFakeAgent) RunWith(_ context.Context, _ string, _ string, _ int, _ agentpkg.RunOptions, _ ...message.Attachment) (<-chan agentpkg.AgentEvent, error) { + return nil, nil +} +func (a *editorFakeAgent) Cancel(_ string) {} +func (a *editorFakeAgent) IsBusy() bool { return a.busy } +func (a *editorFakeAgent) TryLockSession(_ string) bool { return true } +func (a *editorFakeAgent) UnlockSession(_ string) {} +func (a *editorFakeAgent) Update(_ config.AgentName, _ models.ModelID) (models.Model, error) { + return models.Model{}, nil +} +func (a *editorFakeAgent) Summarize(_ context.Context, _ string) error { return nil } +func (a *editorFakeAgent) SummarizeSync(_ context.Context, _ string) error { return nil } +func (a *editorFakeAgent) GenerateRecap(_ context.Context, _ string) (string, error) { return "", nil } + +// ---- helpers ---------------------------------------------------------------- + +// newEditorForTest constructs a minimal editorCmp backed by a stub App. +func newEditorForTest(ctx context.Context, ag *editorFakeAgent) (*editorCmp, *app.App) { + a := app.NewForTest(ctx, ag) + ed := &editorCmp{ + app: a, + session: session.Session{ID: "test-session"}, + textarea: CreateTextArea(nil), + mode: modeNormal, + } + return ed, a +} + +// cmdProducesMsg runs cmd (if non-nil) and returns the produced tea.Msg. +func cmdProducesMsg(cmd tea.Cmd) tea.Msg { + if cmd == nil { + return nil + } + return cmd() +} + +// ---- tests ------------------------------------------------------------------ + +// TestEditor_send_BusyEnqueuesNoWarnCmd asserts that send() while the session +// is busy enqueues the message, resets the textarea, and returns nil (no +// warning cmd). +func TestEditor_send_BusyEnqueuesNoWarnCmd(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + ag := &editorFakeAgent{busy: true} + ed, a := newEditorForTest(ctx, ag) + + ed.textarea.SetValue("hello world") + cmd := ed.send() + // Must be nil — no warning toast for queued submissions. + if cmd != nil { + msg := cmdProducesMsg(cmd) + t.Errorf("send() while busy returned non-nil cmd producing %T: %+v", msg, msg) + } + // Text must have moved into the queue. + if n := a.QueueLen("test-session"); n != 1 { + t.Errorf("QueueLen = %d, want 1", n) + } + // Textarea must be reset. + if v := ed.textarea.Value(); v != "" { + t.Errorf("textarea not reset, still has %q", v) + } + // Stop drain worker started by EnqueueMessage. + a.ShutdownQueues() +} + +// TestEditor_send_EmptyWhileBusyIsNoOp asserts that an empty submit while +// busy is silently discarded (no enqueue, no cmd). +func TestEditor_send_EmptyWhileBusyIsNoOp(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + ag := &editorFakeAgent{busy: true} + ed, a := newEditorForTest(ctx, ag) + + ed.textarea.SetValue("") + cmd := ed.send() + + if cmd != nil { + t.Errorf("empty send() should return nil, got %T", cmd) + } + if n := a.QueueLen("test-session"); n != 0 { + t.Errorf("empty submit should not enqueue, QueueLen = %d", n) + } +} + +// TestEditor_send_IdleDirectDispatch asserts that when the queue is empty AND +// the session is idle, send() dispatches directly (returns a SendMsg cmd) +// without calling EnqueueMessage. +func TestEditor_send_IdleDirectDispatch(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + ag := &editorFakeAgent{busy: false} + ed, a := newEditorForTest(ctx, ag) + + ed.textarea.SetValue("direct message") + cmd := ed.send() + + // Must not enqueue. + if n := a.QueueLen("test-session"); n != 0 { + t.Errorf("idle send enqueued unexpectedly, QueueLen = %d", n) + } + // Must produce a SendMsg. + if cmd == nil { + t.Fatal("idle send returned nil cmd, expected SendMsg") + } + msg := cmdProducesMsg(cmd) + sendMsg, ok := msg.(SendMsg) + if !ok { + t.Fatalf("cmd produced %T, want SendMsg", msg) + } + if sendMsg.Text != "direct message" { + t.Errorf("SendMsg.Text = %q, want %q", sendMsg.Text, "direct message") + } +} + +// TestEditor_send_QueueNonEmptySessionIdleEnqueues asserts FIFO routing: +// when the queue is non-empty but the session is momentarily idle, send() +// enqueues rather than dispatching directly. +func TestEditor_send_QueueNonEmptySessionIdleEnqueues(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + ag := &editorFakeAgent{busy: false} // observably idle + ed, a := newEditorForTest(ctx, ag) + + // Pre-load the queue with an existing message (simulates a message + // already enqueued while busy). + a.EnqueueMessage("test-session", app.QueuedMessage{Text: "existing"}) + a.ShutdownQueues() // stop the worker so it doesn't race + + // Reset the queue state but keep the queued message. + // We need to prevent the drain worker from interfering. + // Reinitialize to have the message in queue with no active worker. + ctx2, cancel2 := context.WithCancel(context.Background()) + defer cancel2() + a2 := app.NewForTest(ctx2, ag) + a2.EnqueueMessage("test-session", app.QueuedMessage{Text: "existing"}) + a2.ShutdownQueues() // kill the worker immediately + + ed2 := &editorCmp{ + app: a2, + session: session.Session{ID: "test-session"}, + textarea: CreateTextArea(nil), + mode: modeNormal, + } + + // Queue has 1 message; session is idle. send() must enqueue, not dispatch. + ed2.textarea.SetValue("new message") + cmd := ed2.send() + + if cmd != nil { + msg := cmdProducesMsg(cmd) + if _, ok := msg.(SendMsg); ok { + t.Error("send() dispatched directly despite non-empty queue (FIFO violation)") + } + } + if n := a2.QueueLen("test-session"); n < 2 { + t.Errorf("expected ≥2 queued messages (existing + new), got %d", n) + } + + a2.ShutdownQueues() + _ = ed +} + +// TestEditor_send_IdlePathErrorSurfaces asserts that when the session is idle +// and agent.Run returns a non-ErrSessionBusy error, the send() function +// still returns a SendMsg cmd (which the chat page's sendMessage will dispatch; +// sendMessage itself surfaces the error). This test validates the editor's role +// which is only to route to direct dispatch — error surfacing happens upstream. +func TestEditor_send_IdlePathErrorSurfaces(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + ag := &editorFakeAgent{busy: false, runErr: errors.New("api error")} + ed, _ := newEditorForTest(ctx, ag) + + ed.textarea.SetValue("direct with error") + cmd := ed.send() + + // The editor must return a SendMsg cmd — error surfacing is chat page's job. + if cmd == nil { + t.Fatal("idle send with run error returned nil cmd") + } + msg := cmdProducesMsg(cmd) + if _, ok := msg.(SendMsg); !ok { + t.Errorf("idle path produced %T, want SendMsg", msg) + } +} diff --git a/internal/tui/components/chat/list.go b/internal/tui/components/chat/list.go index 73bedff4ab..f2a10db38f 100644 --- a/internal/tui/components/chat/list.go +++ b/internal/tui/components/chat/list.go @@ -9,6 +9,7 @@ import ( "charm.land/bubbles/v2/viewport" tea "charm.land/bubbletea/v2" "charm.land/lipgloss/v2" + "github.com/charmbracelet/x/ansi" "github.com/opencode-ai/opencode/internal/app" "github.com/opencode-ai/opencode/internal/llm/agent" "github.com/opencode-ai/opencode/internal/message" @@ -156,6 +157,15 @@ func (m *messagesCmp) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if m.hasCacheMisses() { cmds = append(cmds, m.renderViewAsync()) } + case app.DrainEvent: + // Queue state changed — re-render the banner (queueBanner queries + // app.QueueLen in View, no local state needed). + if msg.SessionID == m.session.ID { + if m.rendering { + m.rendering = false + } + m.renderViewSync() + } case pubsub.Event[session.Session]: if msg.Type == pubsub.UpdatedEvent && msg.Payload.ID == m.session.ID { m.session = msg.Payload @@ -563,7 +573,7 @@ func (m *messagesCmp) View() tea.View { lipgloss.Top, content, "", - m.help(), + m.footer(), ), )) } @@ -575,7 +585,7 @@ func (m *messagesCmp) View() tea.View { lipgloss.Top, m.viewport.View(), m.working(), - m.help(), + m.footer(), ), )) } @@ -708,11 +718,63 @@ func (m *messagesCmp) help() string { baseStyle.Foreground(t.TextMuted()).Bold(true).Render(" for shell"), ) } - return baseStyle. + return text +} + +// footer renders the status row below the working spinner: the queue +// affordance while messages are waiting, otherwise the key help. +// +// It MUST render exactly one line. The viewport is sized to m.height-2 (see +// SetSize), so this component may only emit two further rows — working() and +// footer(). A third row overflows the container's MaxHeight and the bottom +// line is silently clipped; that is how the queue banner used to swallow the +// help bar for the rest of the session. Overlong text is therefore truncated +// rather than wrapped. +func (m *messagesCmp) footer() string { + text := m.queueBanner() + if text == "" { + text = m.help() + } + if m.width > 0 { + text = ansi.Truncate(text, m.width, "…") + } + return styles.BaseStyle(). Width(m.width). + MaxHeight(1). Render(text) } +// queueBanner renders the in-memory queue affordance when messages are +// waiting, replacing the help bar for as long as the queue is non-empty. It is +// styled distinctly from persisted chat messages (muted colour, no chat +// bubble) and advertises both queue keys: ctrl+g to inspect the queued text, +// ctrl+x to discard it. Returns "" when nothing is queued. +func (m *messagesCmp) queueBanner() string { + if m.session.ID == "" { + return "" + } + n := m.app.QueueLen(m.session.ID) + if n == 0 { + return "" + } + t := theme.CurrentTheme() + baseStyle := styles.BaseStyle() + noun := "message" + if n != 1 { + noun = "messages" + } + hints := "ctrl+g to view, ctrl+x to discard" + if m.app.ActiveAgent().IsBusy() { + // The help bar is hidden while the banner is up, so keep the cancel + // hint reachable — a queue almost always means a running request. + hints += ", esc to cancel" + } + return baseStyle. + Foreground(t.TextMuted()). + Italic(true). + Render(fmt.Sprintf("%d %s queued — press %s", n, noun, hints)) +} + func (m *messagesCmp) initialScreen() string { baseStyle := styles.BaseStyle() diff --git a/internal/tui/components/chat/list_queue_test.go b/internal/tui/components/chat/list_queue_test.go new file mode 100644 index 0000000000..be69f89bd8 --- /dev/null +++ b/internal/tui/components/chat/list_queue_test.go @@ -0,0 +1,113 @@ +package chat + +import ( + "context" + "strings" + "testing" + + "charm.land/bubbles/v2/spinner" + "charm.land/bubbles/v2/viewport" + "charm.land/lipgloss/v2" + "github.com/opencode-ai/opencode/internal/app" + "github.com/opencode-ai/opencode/internal/message" + "github.com/opencode-ai/opencode/internal/session" +) + +// newMessagesForTest builds a messagesCmp with one rendered message so View +// takes the normal (non-empty) branch, sized like the real container. +func newMessagesForTest(t *testing.T, a *app.App, width int) *messagesCmp { + t.Helper() + vp := viewport.New() + m := &messagesCmp{ + app: a, + cachedContent: make(map[string]cacheItem), + taskMessages: make(map[string][]message.Message), + viewport: vp, + spinner: spinner.New(), + attachments: viewport.New(), + session: session.Session{ID: "test-session"}, + messages: []message.Message{{ + ID: "m1", + SessionID: "test-session", + Role: message.User, + }}, + } + m.width = width + m.height = 12 + m.viewport.SetWidth(m.width) + m.viewport.SetHeight(m.height - 2) + return m +} + +// The chat view must never render more rows than it was given: the container +// applies MaxHeight, so an extra row silently clips the bottom line. That is +// how the queue banner made the help bar vanish for the rest of the session. +func TestMessages_ViewHeightFitsRegardlessOfQueue(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + ag := &editorFakeAgent{busy: true} + a := app.NewForTest(ctx, ag) + + m := newMessagesForTest(t, a, 80) + + if h := lipgloss.Height(m.View().Content); h != m.height { + t.Fatalf("empty queue: view height = %d, want %d", h, m.height) + } + + a.EnqueueForTest("test-session", app.QueuedMessage{Text: "queued one"}) + if h := lipgloss.Height(m.View().Content); h != m.height { + t.Fatalf("non-empty queue: view height = %d, want %d", h, m.height) + } +} + +// The help bar is the affordance that teaches enter / \ / / / ! — it must be +// visible whenever nothing is queued, and come back after a drain. +func TestMessages_HelpVisibleWhenQueueEmpty(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + ag := &editorFakeAgent{} + a := app.NewForTest(ctx, ag) + + // 100 columns fits the full help line; narrower terminals truncate its tail. + m := newMessagesForTest(t, a, 100) + + view := m.View().Content + if !strings.Contains(view, "for shell") { + t.Errorf("help bar missing while queue is empty:\n%s", view) + } + + a.EnqueueForTest("test-session", app.QueuedMessage{Text: "queued one"}) + view = m.View().Content + if !strings.Contains(view, "queued") { + t.Errorf("queue banner missing while a message is queued:\n%s", view) + } + if !strings.Contains(view, "ctrl+g") || !strings.Contains(view, "ctrl+x") { + t.Errorf("queue banner must advertise ctrl+g / ctrl+x:\n%s", view) + } + + a.DiscardQueue("test-session") + view = m.View().Content + if !strings.Contains(view, "for shell") { + t.Errorf("help bar did not come back after the queue drained:\n%s", view) + } +} + +// A queued message with newlines must not grow the banner into a second row. +func TestMessages_FooterIsSingleLine(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + ag := &editorFakeAgent{busy: true} + a := app.NewForTest(ctx, ag) + + m := newMessagesForTest(t, a, 80) + for i := 0; i < 40; i++ { + a.EnqueueForTest("test-session", app.QueuedMessage{Text: "line\nline\nline"}) + } + + if h := lipgloss.Height(m.footer()); h != 1 { + t.Fatalf("footer height = %d, want 1", h) + } +} diff --git a/internal/tui/components/dialog/queue.go b/internal/tui/components/dialog/queue.go new file mode 100644 index 0000000000..967b7e1dbf --- /dev/null +++ b/internal/tui/components/dialog/queue.go @@ -0,0 +1,210 @@ +package dialog + +import ( + "fmt" + "strings" + + "charm.land/bubbles/v2/key" + tea "charm.land/bubbletea/v2" + "charm.land/lipgloss/v2" + "github.com/charmbracelet/x/ansi" + + "github.com/opencode-ai/opencode/internal/app" + "github.com/opencode-ai/opencode/internal/tui/layout" + "github.com/opencode-ai/opencode/internal/tui/styles" + "github.com/opencode-ai/opencode/internal/tui/theme" + "github.com/opencode-ai/opencode/internal/tui/util" +) + +// ToggleQueueDialogMsg asks the TUI to show or hide the queued-messages viewer +// for a session. Emitted by the chat page (ctrl+g). +type ToggleQueueDialogMsg struct { + SessionID string +} + +// CloseQueueDialogMsg closes the queued-messages viewer. +type CloseQueueDialogMsg struct{} + +// QueueDialog previews the messages waiting in a session's in-memory queue. +// It is a viewer: the only mutation it offers is discarding the whole queue +// (ctrl+x), mirroring the chat page's binding. +type QueueDialog interface { + tea.Model + layout.Bindings + SetSession(sessionID string) + SetSize(width, height int) +} + +type queueDialogCmp struct { + app *app.App + sessionID string + width, height int +} + +var queueDialogKeys = struct { + Close key.Binding + Discard key.Binding +}{ + Close: key.NewBinding( + key.WithKeys("esc", "ctrl+g", "q"), + key.WithHelp("esc", "close"), + ), + Discard: key.NewBinding( + key.WithKeys("ctrl+x"), + key.WithHelp("ctrl+x", "discard queued messages"), + ), +} + +const ( + // queueDialogMaxWidth caps the dialog so long pasted messages don't stretch + // it across a wide terminal. + queueDialogMaxWidth = 76 + // queueDialogMaxEntries caps the listed previews; the rest is summarised. + queueDialogMaxEntries = 12 +) + +func (d *queueDialogCmp) SetSession(sessionID string) { + d.sessionID = sessionID +} + +func (d *queueDialogCmp) SetSize(width, height int) { + d.width, d.height = width, height +} + +func (d *queueDialogCmp) Init() tea.Cmd { + return nil +} + +func (d *queueDialogCmp) Update(msg tea.Msg) (tea.Model, tea.Cmd) { + switch msg := msg.(type) { + case tea.KeyPressMsg: + switch { + case key.Matches(msg, queueDialogKeys.Close): + return d, util.CmdHandler(CloseQueueDialogMsg{}) + case key.Matches(msg, queueDialogKeys.Discard): + // The dialog swallows key presses, so the chat page's ctrl+x never + // fires while it is open — handle it here so the advertised key + // works from inside the viewer. + if d.sessionID != "" { + d.app.DiscardQueue(d.sessionID) + } + return d, util.CmdHandler(CloseQueueDialogMsg{}) + } + } + return d, nil +} + +func (d *queueDialogCmp) View() tea.View { + t := theme.CurrentTheme() + baseStyle := styles.BaseStyle() + bg := t.Background() + + // Read the queue at render time rather than caching it: the drain worker + // keeps delivering while the dialog is open, so a snapshot taken on open + // would show messages that have already been sent. + var queued []app.QueuedMessage + if d.sessionID != "" { + queued = d.app.QueuedMessages(d.sessionID) + } + + innerWidth := queueDialogMaxWidth + if d.width > 0 && d.width-10 < innerWidth { + innerWidth = max(20, d.width-10) + } + + header := baseStyle. + Foreground(t.Primary()). + Bold(true). + Render(fmt.Sprintf("Queued messages (%d)", len(queued))) + + lines := []string{header, ""} + + if len(queued) == 0 { + lines = append(lines, baseStyle.Foreground(t.TextMuted()).Italic(true). + Render("Nothing queued — messages typed while the agent is busy land here.")) + } else { + shown := queued + if len(shown) > d.maxEntries() { + shown = shown[:d.maxEntries()] + } + for i, m := range shown { + lines = append(lines, queueEntryLine(i, m, innerWidth, baseStyle, t)) + } + if rest := len(queued) - len(shown); rest > 0 { + lines = append(lines, baseStyle.Foreground(t.TextMuted()).Italic(true). + Render(fmt.Sprintf("… and %d more", rest))) + } + } + + lines = append(lines, + "", + baseStyle.Foreground(t.TextMuted()).Render("esc close · ctrl+x discard all"), + ) + + content := baseStyle.Width(innerWidth).Render( + lipgloss.JoinVertical(lipgloss.Left, lines...), + ) + + rendered := baseStyle.Padding(1, 2). + Border(lipgloss.RoundedBorder()). + BorderBackground(bg). + BorderForeground(t.TextMuted()). + Width(innerWidth + 6). + Render(content) + + return tea.NewView(styles.ForceReplaceBackgroundWithLipgloss(rendered, bg)) +} + +// maxEntries is how many previews fit: the hard cap, reduced further on short +// terminals so the dialog cannot outgrow the window it is centred in. The +// subtracted rows are the border (2), padding (2), header (2) and footer (2). +func (d *queueDialogCmp) maxEntries() int { + n := queueDialogMaxEntries + if d.height > 0 { + if avail := d.height - 8; avail < n { + n = max(1, avail) + } + } + return n +} + +// queueEntryLine renders one queued message as a single truncated preview line +// prefixed with its delivery position. +func queueEntryLine(idx int, m app.QueuedMessage, width int, baseStyle lipgloss.Style, t theme.Theme) string { + prefix := fmt.Sprintf("%d. ", idx+1) + suffix := "" + if n := len(m.Attachments); n > 0 { + suffix = fmt.Sprintf(" (+%d attachment%s)", n, plural(n)) + } + + // Collapse whitespace so a multi-line message stays one row, and drop any + // escape sequences the text may carry so it cannot repaint the dialog. + preview := strings.Join(strings.Fields(ansi.Strip(m.Text)), " ") + if preview == "" { + preview = "(empty)" + } + budget := max(8, width-lipgloss.Width(prefix)-lipgloss.Width(suffix)) + preview = ansi.Truncate(preview, budget, "…") + + return lipgloss.JoinHorizontal( + lipgloss.Left, + baseStyle.Foreground(t.TextMuted()).Render(prefix), + baseStyle.Foreground(t.Text()).Render(preview), + baseStyle.Foreground(t.TextMuted()).Render(suffix), + ) +} + +func plural(n int) string { + if n == 1 { + return "" + } + return "s" +} + +func (d *queueDialogCmp) BindingKeys() []key.Binding { + return []key.Binding{queueDialogKeys.Close, queueDialogKeys.Discard} +} + +func NewQueueDialogCmp(app *app.App) QueueDialog { + return &queueDialogCmp{app: app} +} diff --git a/internal/tui/components/dialog/queue_test.go b/internal/tui/components/dialog/queue_test.go new file mode 100644 index 0000000000..ca75eecded --- /dev/null +++ b/internal/tui/components/dialog/queue_test.go @@ -0,0 +1,134 @@ +package dialog + +import ( + "context" + "strings" + "testing" + + tea "charm.land/bubbletea/v2" + "github.com/charmbracelet/x/ansi" + "github.com/opencode-ai/opencode/internal/app" + "github.com/opencode-ai/opencode/internal/tui/styles" + "github.com/opencode-ai/opencode/internal/tui/theme" +) + +func newQueueDialogForTest(t *testing.T, a *app.App) *queueDialogCmp { + t.Helper() + d := NewQueueDialogCmp(a).(*queueDialogCmp) + d.SetSize(120, 40) + d.SetSession("s1") + return d +} + +func TestQueueDialog_ListsQueuedMessagesInOrder(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + a := app.NewForTest(ctx, nil) + a.EnqueueForTest("s1", app.QueuedMessage{Text: "first message"}) + a.EnqueueForTest("s1", app.QueuedMessage{Text: "second message"}) + + d := newQueueDialogForTest(t, a) + view := ansi.Strip(d.View().Content) + + if !strings.Contains(view, "Queued messages (2)") { + t.Errorf("header missing count:\n%s", view) + } + first := strings.Index(view, "first message") + second := strings.Index(view, "second message") + if first < 0 || second < 0 { + t.Fatalf("previews missing:\n%s", view) + } + if first > second { + t.Errorf("previews out of delivery order:\n%s", view) + } +} + +// A multi-line or escape-laden paste must stay a single bounded preview row. +func TestQueueDialog_PreviewIsSingleBoundedLine(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + a := app.NewForTest(ctx, nil) + a.EnqueueForTest("s1", app.QueuedMessage{ + Text: "line one\nline two\x1b[31m red \x1b[0m" + strings.Repeat("x", 500), + }) + + line := queueEntryLine(0, a.QueuedMessages("s1")[0], 60, styles.BaseStyle(), theme.CurrentTheme()) + + if got := strings.Count(line, "\n"); got != 0 { + t.Errorf("preview spans %d extra lines: %q", got, line) + } + if w := ansi.StringWidth(line); w > 60 { + t.Errorf("preview width = %d, want <= 60", w) + } + if strings.Contains(ansi.Strip(line), "\x1b") { + t.Errorf("escape sequences survived: %q", line) + } +} + +func TestQueueDialog_EmptyState(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + d := newQueueDialogForTest(t, app.NewForTest(ctx, nil)) + view := ansi.Strip(d.View().Content) + + if !strings.Contains(view, "Queued messages (0)") || !strings.Contains(view, "Nothing queued") { + t.Errorf("empty state not rendered:\n%s", view) + } +} + +func TestQueueDialog_CloseKeys(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + d := newQueueDialogForTest(t, app.NewForTest(ctx, nil)) + + for _, k := range []string{"esc", "ctrl+g", "q"} { + _, cmd := d.Update(keyPress(k)) + if cmd == nil { + t.Fatalf("%s produced no cmd", k) + } + if _, ok := cmd().(CloseQueueDialogMsg); !ok { + t.Errorf("%s did not close the dialog, got %T", k, cmd()) + } + } +} + +// ctrl+x is advertised in the dialog footer, and the dialog swallows key +// presses — so it must discard the queue itself. +func TestQueueDialog_DiscardKeyEmptiesQueue(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + a := app.NewForTest(ctx, nil) + a.EnqueueForTest("s1", app.QueuedMessage{Text: "doomed"}) + + d := newQueueDialogForTest(t, a) + _, cmd := d.Update(keyPress("ctrl+x")) + + if n := a.QueueLen("s1"); n != 0 { + t.Errorf("QueueLen after ctrl+x = %d, want 0", n) + } + if cmd == nil { + t.Fatal("ctrl+x produced no cmd") + } + if _, ok := cmd().(CloseQueueDialogMsg); !ok { + t.Errorf("ctrl+x did not close the dialog, got %T", cmd()) + } +} + +func keyPress(k string) tea.KeyPressMsg { + switch k { + case "esc": + return tea.KeyPressMsg{Code: tea.KeyEscape} + case "q": + return tea.KeyPressMsg{Code: 'q', Text: "q"} + case "ctrl+g": + return tea.KeyPressMsg{Code: 'g', Mod: tea.ModCtrl} + case "ctrl+x": + return tea.KeyPressMsg{Code: 'x', Mod: tea.ModCtrl} + } + panic("unhandled key " + k) +} diff --git a/internal/tui/page/chat.go b/internal/tui/page/chat.go index d531f2aa58..61e3e50c25 100644 --- a/internal/tui/page/chat.go +++ b/internal/tui/page/chat.go @@ -2,6 +2,7 @@ package page import ( "context" + "errors" "fmt" "path/filepath" "sort" @@ -16,6 +17,7 @@ import ( "github.com/opencode-ai/opencode/internal/completions" "github.com/opencode-ai/opencode/internal/config" "github.com/opencode-ai/opencode/internal/format" + "github.com/opencode-ai/opencode/internal/llm/agent" "github.com/opencode-ai/opencode/internal/llm/tools" "github.com/opencode-ai/opencode/internal/logging" "github.com/opencode-ai/opencode/internal/message" @@ -57,6 +59,16 @@ type ChatKeyMap struct { ShowCommandCompletionDialog key.Binding NewSession key.Binding Cancel key.Binding + // DiscardQueue clears all queued messages for the active session. + // Key chosen: ctrl+x — absent from editorMaps (enter/ctrl+s, ctrl+e), + // DeleteKeyMaps (ctrl+r, esc, r), messageKeys (pgdown, pgup, ctrl+u, + // ctrl+d), and the bubbles v2 textarea default KeyMap. + DiscardQueue key.Binding + // ShowQueue toggles the read-only queued-messages viewer. + // Key chosen: ctrl+g — free across the app keymap and, unlike ctrl+m + // (CR/enter) or ctrl+i (tab), it is not an alias of another key in + // terminals without the kitty keyboard protocol. + ShowQueue key.Binding } var keyMap = ChatKeyMap{ @@ -76,6 +88,14 @@ var keyMap = ChatKeyMap{ key.WithKeys("esc"), key.WithHelp("esc", "cancel"), ), + DiscardQueue: key.NewBinding( + key.WithKeys("ctrl+x"), + key.WithHelp("ctrl+x", "discard queued messages"), + ), + ShowQueue: key.NewBinding( + key.WithKeys("ctrl+g"), + key.WithHelp("ctrl+g", "view queued messages"), + ), } func (p *chatPage) Init() tea.Cmd { @@ -157,6 +177,23 @@ func (p *chatPage) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return p, nil case chat.ShellResultMsg: cmds = append(cmds, p.handleShellResult(msg)) + case app.DrainEvent: + // Drain-worker notification: update queue affordance and surface errors. + // The error is surfaced even for a session the user is not currently + // viewing: a halted drain is terminal (the worker is gone and the queue + // is stalled until ctrl+x or a new submit), the banner looks identical + // to a healthy mid-drain queue, and the event is never re-emitted — so + // filtering on the active session drops the only signal there is. + if msg.Err != nil { + if msg.SessionID == p.session.ID { + cmds = append(cmds, util.ReportError(msg.Err)) + } else { + cmds = append(cmds, util.ReportError( + fmt.Errorf("session %s: %w", msg.SessionID, msg.Err))) + } + } + // Fall through: let the message reach the messages component so it + // re-renders the queue banner (list.go queries app.QueueLen in View). case chat.SendMsg: if resolved := p.resolveInlineSlash(msg.Text); resolved != nil { return p, resolved @@ -166,6 +203,8 @@ func (p *chatPage) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return p, cmd } case dialog.CommandRunCustomMsg: + // Queuing slash-command / custom-command runs is a future decision; + // retain the busy-reject guard unchanged (task 6.2). if p.app.ActiveAgent().IsBusy() { return p, util.ReportWarn("Agent is busy, please wait before executing a command...") } @@ -248,6 +287,19 @@ func (p *chatPage) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return p, cmd case tea.KeyPressMsg: switch { + case key.Matches(msg, keyMap.ShowQueue): + // Toggle the queue viewer. Handled even with an empty queue so the + // dialog can explain what the queue is; it closes on esc/ctrl+g. + if p.session.ID != "" { + return p, util.CmdHandler(dialog.ToggleQueueDialogMsg{SessionID: p.session.ID}) + } + case key.Matches(msg, keyMap.DiscardQueue): + // Discard all queued messages for the active session. The queue + // survives Esc (which only cancels the in-flight run); this key is + // the explicit discard action (Decision 4). + if p.session.ID != "" && p.app.QueueLen(p.session.ID) > 0 { + p.app.DiscardQueue(p.session.ID) + } case key.Matches(msg, keyMap.Cancel): // In shell mode, ESC should exit shell mode (handled by editor) if p.shellMode { @@ -261,7 +313,8 @@ func (p *chatPage) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if p.vimMode == "INSERT" { break } - // In vim NORMAL mode or no vim: cancel running request if agent is busy + // In vim NORMAL mode or no vim: cancel running request if agent is busy. + // Queued messages survive Esc by design (Decision 4); use ctrl+x to discard. if p.session.ID != "" && p.app.ActiveAgent().IsBusy() { p.app.ActiveAgent().Cancel(p.session.ID) return p, nil @@ -451,6 +504,20 @@ func (p *chatPage) sendMessage(text string, attachments []message.Attachment) te _, err = p.app.ActiveAgent().Run(context.Background(), p.session.ID, text, 0, attachments...) if err != nil { + // ErrSessionBusy on the direct idle path means the editor's + // queue-empty + not-busy check lost a race (a drain worker, cron, + // flow step or bridge dispatch claimed the slot in between). The + // submission is NOT surfaced as an error toast — but it must not be + // dropped either: the textarea has already been reset, so returning + // here would silently discard the user's text. Hand it to the queue + // so the drain worker retries it (task 4.1). + if errors.Is(err, agent.ErrSessionBusy) { + p.app.EnqueueMessage(p.session.ID, app.QueuedMessage{ + Text: text, + Attachments: attachments, + }) + return tea.Batch(cmds...) + } return util.ReportError(err) } return tea.Batch(cmds...) diff --git a/internal/tui/page/chat_sendmessage_test.go b/internal/tui/page/chat_sendmessage_test.go new file mode 100644 index 0000000000..19fea1d0dc --- /dev/null +++ b/internal/tui/page/chat_sendmessage_test.go @@ -0,0 +1,131 @@ +package page + +import ( + "context" + "sync" + "testing" + + agentpkg "github.com/opencode-ai/opencode/internal/llm/agent" + "github.com/opencode-ai/opencode/internal/llm/models" + "github.com/opencode-ai/opencode/internal/llm/tools" + "github.com/opencode-ai/opencode/internal/message" + "github.com/opencode-ai/opencode/internal/pubsub" + "github.com/opencode-ai/opencode/internal/session" + "github.com/opencode-ai/opencode/internal/tui/util" + + "github.com/opencode-ai/opencode/internal/app" + "github.com/opencode-ai/opencode/internal/config" +) + +// ---- fake agent ------------------------------------------------------------- + +// busyFakeAgent returns runErr from every Run call. Only Run and IsSessionBusy +// are exercised; the remaining methods exist to satisfy agent.Service. +type busyFakeAgent struct { + runErr error +} + +func (a *busyFakeAgent) IsSessionBusy(_ string) bool { return false } + +func (a *busyFakeAgent) Run(_ context.Context, _ string, _ string, _ int, _ ...message.Attachment) (<-chan agentpkg.AgentEvent, error) { + if a.runErr != nil { + return nil, a.runErr + } + ch := make(chan agentpkg.AgentEvent) + close(ch) + return ch, nil +} + +func (a *busyFakeAgent) Subscribe(_ context.Context) <-chan pubsub.Event[agentpkg.AgentEvent] { + ch := make(chan pubsub.Event[agentpkg.AgentEvent]) + close(ch) + return ch +} +func (a *busyFakeAgent) AgentID() config.AgentName { return "" } +func (a *busyFakeAgent) Model() models.Model { return models.Model{} } +func (a *busyFakeAgent) Tools() []tools.BaseTool { return nil } +func (a *busyFakeAgent) ResolvedTools() ([]tools.BaseTool, bool) { return nil, false } +func (a *busyFakeAgent) RunWith(_ context.Context, _ string, _ string, _ int, _ agentpkg.RunOptions, _ ...message.Attachment) (<-chan agentpkg.AgentEvent, error) { + return nil, nil +} +func (a *busyFakeAgent) Cancel(_ string) {} +func (a *busyFakeAgent) IsBusy() bool { return false } +func (a *busyFakeAgent) TryLockSession(_ string) bool { return true } +func (a *busyFakeAgent) UnlockSession(_ string) {} +func (a *busyFakeAgent) Update(_ config.AgentName, _ models.ModelID) (models.Model, error) { + return models.Model{}, nil +} +func (a *busyFakeAgent) Summarize(_ context.Context, _ string) error { return nil } +func (a *busyFakeAgent) SummarizeSync(_ context.Context, _ string) error { return nil } +func (a *busyFakeAgent) GenerateRecap(_ context.Context, _ string) (string, error) { return "", nil } + +// ---- tests ------------------------------------------------------------------ + +// TestChatPage_sendMessage_BusyRaceEnqueues covers the direct-dispatch race: +// the editor routes to the queue only when it observes queue-empty AND +// not-busy, but another actor (drain worker, cron, flow step, bridge dispatch) +// can claim the session slot between that check and agent.Run. The resulting +// ErrSessionBusy must NOT be surfaced as an error toast (it is retryable) and +// must NOT be dropped — the textarea has already been reset, so dropping it +// silently loses the user's submission. +func TestChatPage_sendMessage_BusyRaceEnqueues(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + ag := &busyFakeAgent{runErr: agentpkg.ErrSessionBusy} + a := app.NewForTest(ctx, ag) + + // Capture drain events synchronously: EnqueueMessage notifies with the + // post-append length before returning, so this is race-free — unlike + // polling QueueLen, which the drain worker mutates concurrently. + var mu sync.Mutex + var events []app.DrainEvent + a.SetDrainNotifier(func(e app.DrainEvent) { + mu.Lock() + defer mu.Unlock() + events = append(events, e) + }) + + p := &chatPage{app: a, session: session.Session{ID: "s1"}} + + cmd := p.sendMessage("redirect me", nil) + a.ShutdownQueues() + + // No error toast for a retryable busy race. + if cmd != nil { + if msg := cmd(); msg != nil { + if _, isErr := msg.(util.InfoMsg); isErr { + t.Errorf("sendMessage surfaced an info/error toast on ErrSessionBusy: %+v", msg) + } + } + } + + mu.Lock() + defer mu.Unlock() + if len(events) == 0 { + t.Fatal("no DrainEvent emitted — the message was dropped instead of enqueued") + } + if events[0].SessionID != "s1" || events[0].QueueLen != 1 { + t.Errorf("first DrainEvent = %+v, want {SessionID: s1, QueueLen: 1}", events[0]) + } +} + +// TestChatPage_sendMessage_NonBusyErrorStillSurfaces guards the other half of +// the branch: a real failure must still reach the user rather than being +// swallowed into the queue. +func TestChatPage_sendMessage_NonBusyErrorStillSurfaces(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + ag := &busyFakeAgent{runErr: context.DeadlineExceeded} + a := app.NewForTest(ctx, ag) + p := &chatPage{app: a, session: session.Session{ID: "s1"}} + + cmd := p.sendMessage("hello", nil) + if cmd == nil { + t.Fatal("non-busy error produced no cmd — the failure was swallowed") + } + if n := a.QueueLen("s1"); n != 0 { + t.Errorf("QueueLen = %d, want 0 (non-busy errors must not enqueue)", n) + } +} diff --git a/internal/tui/tui.go b/internal/tui/tui.go index 4265a34bb2..cda6a2ec46 100644 --- a/internal/tui/tui.go +++ b/internal/tui/tui.go @@ -180,6 +180,9 @@ type appModel struct { showMissedCronDialog bool missedCronDialog dialog.MissedCronDialog + showQueueDialog bool + queueDialog dialog.QueueDialog + showQuestionDialog bool questionDialog dialog.QuestionDialogCmp @@ -218,6 +221,8 @@ func (a appModel) Init() tea.Cmd { cmds = append(cmds, cmd) cmd = a.missedCronDialog.Init() cmds = append(cmds, cmd) + cmd = a.queueDialog.Init() + cmds = append(cmds, cmd) cmd = a.questionDialog.Init() cmds = append(cmds, cmd) @@ -280,6 +285,7 @@ func (a appModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { cmds = append(cmds, filepickerCmd) a.initDialog.SetSize(msg.Width, msg.Height) + a.queueDialog.SetSize(msg.Width, msg.Height) if a.showMultiArgumentsDialog { a.multiArgumentsDialog.SetSize(msg.Width, msg.Height) @@ -465,6 +471,20 @@ func (a appModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case sessionsCleanupFailedMsg: return a, util.ReportError(msg.err) + case dialog.ToggleQueueDialogMsg: + // ctrl+g from the chat page. Toggling here covers the closed→open + // direction; while the dialog is open it swallows key presses and + // closes itself via CloseQueueDialogMsg. + a.showQueueDialog = !a.showQueueDialog + if a.showQueueDialog { + a.queueDialog.SetSession(msg.SessionID) + } + return a, nil + + case dialog.CloseQueueDialogMsg: + a.showQueueDialog = false + return a, nil + case pubsub.Event[cron.MissedOneShotsEvent]: // Surface missed one-shots as a confirmation dialog. The scheduler // publishes this once at startup; jobs queue up if the dialog is @@ -631,6 +651,11 @@ func (a appModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { a.dismissQuestionDialog() a.selectedSession = msg a.app.SetActiveSessionID(msg.ID) + // A human is looking at this session now, so a `question` raised + // here must open the dialog rather than be auto-answered — the cron + // scheduler marks sessions whose jobs fire unwatched as unattended + // (see cron.Scheduler.fireJob). + a.app.Permissions.RemoveUnattendedSession(msg.ID) a.sessionDialog.SetSelectedSession(msg.ID) tb, _ := a.topbar.Update(msg) a.topbar = tb.(core.TopBarCmp) @@ -824,7 +849,7 @@ func (a appModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { !a.showSessionDialog && !a.showDeleteSessionDialog && !a.showCommandDialog && !a.showModelDialog && !a.showFilepicker && !a.showThemeDialog && !a.showHelp && !a.showInitDialog && !a.showMultiArgumentsDialog && - !a.isCompacting && !a.app.ActiveAgent().IsBusy() && + !a.showQueueDialog && !a.isCompacting && !a.app.ActiveAgent().IsBusy() && !a.pageHasActiveOverlay() { agentName := a.app.SwitchAgent() return a, tea.Batch( @@ -838,7 +863,7 @@ func (a appModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { !a.showSessionDialog && !a.showDeleteSessionDialog && !a.showCommandDialog && !a.showModelDialog && !a.showFilepicker && !a.showThemeDialog && !a.showHelp && !a.showInitDialog && !a.showMultiArgumentsDialog && - !a.isCompacting && !a.app.ActiveAgent().IsBusy() && + !a.showQueueDialog && !a.isCompacting && !a.app.ActiveAgent().IsBusy() && !a.pageHasActiveOverlay() { agentName := a.app.SwitchAgentReverse() return a, tea.Batch( @@ -1019,6 +1044,15 @@ func (a appModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } } + if a.showQueueDialog { + d, queueCmd := a.queueDialog.Update(msg) + a.queueDialog = d.(dialog.QueueDialog) + cmds = append(cmds, queueCmd) + if _, ok := msg.(tea.KeyPressMsg); ok { + return a, tea.Batch(cmds...) + } + } + switch msg.(type) { case pubsub.Event[agent.MCPServerEvent]: chat.InvalidateMcpCache() @@ -1103,7 +1137,8 @@ func (a *appModel) anyDismissibleDialogOpen() bool { a.showThemeDialog || a.showInitDialog || a.showSessionsCleanupDialog || - a.showMissedCronDialog + a.showMissedCronDialog || + a.showQueueDialog } // dismissAllDialogs closes every dismissible overlay. Intended for ctrl+c @@ -1119,6 +1154,7 @@ func (a *appModel) dismissAllDialogs() { a.showInitDialog = false a.showSessionsCleanupDialog = false a.showMissedCronDialog = false + a.showQueueDialog = false if a.showFilepicker { a.showFilepicker = false a.filepicker.ToggleFilepicker(a.showFilepicker) @@ -1289,6 +1325,10 @@ func (a appModel) View() tea.View { centerOverlay(a.missedCronDialog.View().Content) } + if a.showQueueDialog { + centerOverlay(a.queueDialog.View().Content) + } + v := tea.NewView(appView) v.AltScreen = true v.ReportFocus = true @@ -1327,6 +1367,7 @@ func New(app *app.App) tea.Model { filepicker: dialog.NewFilepickerCmp(app), sessionsCleanupDialog: dialog.NewSessionsCleanupDialogCmp(), missedCronDialog: dialog.NewMissedCronDialog(), + queueDialog: dialog.NewQueueDialogCmp(app), } // Wire the cron scheduler's active-session view to the TUI's selected session. diff --git a/opencode-schema.json b/opencode-schema.json index 747bd24831..70222e1faa 100644 --- a/opencode-schema.json +++ b/opencode-schema.json @@ -983,6 +983,80 @@ "description": "LLM provider configurations", "type": "object" }, + "router": { + "additionalProperties": false, + "description": "Chat-bridge configuration. The bridge connects opencode to Telegram, Slack, and Mattermost. Set at least one channel identity to enable.", + "properties": { + "channels": { + "description": "Per-platform channel configuration. See docs/bridge.md for field details.", + "properties": { + "external": { + "description": "External relay channel configuration.", + "type": "object" + }, + "mattermost": { + "description": "Mattermost channel configuration.", + "type": "object" + }, + "slack": { + "description": "Slack channel configuration.", + "type": "object" + }, + "telegram": { + "description": "Telegram channel configuration.", + "type": "object" + } + }, + "type": "object" + }, + "permissionMode": { + "description": "How permission requests are resolved on bridge-owned sessions: 'allow' auto-approves, 'deny' auto-denies, 'ask' or empty defers to the opencode UI (hangs headless). Unrecognised values fail-safe to deny.", + "enum": [ + "allow", + "deny", + "ask" + ], + "type": "string" + }, + "questionMode": { + "description": "How agent questions are surfaced: 'interactive' renders platform-native UI (buttons/blocks); 'auto-reject' returns the default without prompting; 'disabled' suppresses the question flow.", + "enum": [ + "interactive", + "auto-reject", + "disabled" + ], + "type": "string" + }, + "questionNudgeIntervalSeconds": { + "description": "Idle gap (seconds) after which the bridge re-posts a 'still waiting' nudge to a session with an outstanding question. 0 = built-in default (300 s); \u003c0 = disable nudging.", + "type": "integer" + }, + "questionNudgeMax": { + "description": "Maximum nudges per pending question. 0 = built-in default (3); \u003c0 = unlimited.", + "type": "integer" + }, + "queueAcknowledgementsEnabled": { + "default": false, + "description": "When true, the bridge sends an in-place-editable '⏳ queued' acknowledgement to a sender whose message is enqueued behind an in-flight agent run. The ack is updated as the queue drains and resolved to '▶ Processing…' when the run starts. Disabled by default; enable for reviewers who need visibility into queue depth.", + "type": "boolean" + }, + "toolUpdateVerbosity": { + "default": "compact", + "description": "Detail level when toolUpdatesEnabled is true: 'compact' (default) emits one line per call with glyph, name, and elapsed time; 'full' adds argument and result detail.", + "enum": [ + "compact", + "full" + ], + "type": "string" + }, + "toolUpdatesEnabled": { + "default": false, + "description": "Stream tool-call lifecycle events (pending/running/completed) to the chat surface. Failures always surface regardless of this flag.", + "type": "boolean" + } + }, + "type": "object" + }, "sessionCleanup": { "additionalProperties": false, "description": "Session cleanup configuration for removing old sessions", diff --git a/openspec/changes/bridge-queue-visibility-and-loss-paths/.openspec.yaml b/openspec/changes/bridge-queue-visibility-and-loss-paths/.openspec.yaml new file mode 100644 index 0000000000..34f54d2e6c --- /dev/null +++ b/openspec/changes/bridge-queue-visibility-and-loss-paths/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-09-05 diff --git a/openspec/changes/bridge-queue-visibility-and-loss-paths/design.md b/openspec/changes/bridge-queue-visibility-and-loss-paths/design.md new file mode 100644 index 0000000000..b93ffaa49c --- /dev/null +++ b/openspec/changes/bridge-queue-visibility-and-loss-paths/design.md @@ -0,0 +1,228 @@ +# Design: bridge-queue-visibility-and-loss-paths + +## Context + +See `proposal.md § Why` for motivation. The relevant existing architecture: + +- `sessionDispatch.inbound` is a buffered channel (cap 16, `dispatch.go:30`). `pushInbound` + (`dispatch.go:908-914`) blocks on send with no `default:` branch. The `run()` loop + (`dispatch.go:124-138`) calls `handleInbound` which blocks for the entire `agent.Run` + lifetime. +- On `ErrSessionBusy` today (`dispatch.go:218-223`): the message content is discarded; the + user is told "please resend". `runFailureMessage` (`dispatch.go:258-269`) contains the + lossy wording pinned by `TestRunFailureMessage_BusyDoesNotAdviseAbort` + (`dispatch_test.go:60-74`). +- `runInboundLoop` (`service.go:426-438`) is a **single shared goroutine** for the whole + service. It calls `s.dispatchInbound(ctx, in)` **synchronously** (line 435). + `dispatchInbound` → `pushInbound` blocks when the target session's `d.inbound` is full. + This means: if any one session's channel fills, `runInboundLoop` blocks, and **every + other session on every adapter stops being dispatched**. This is the cross-session + starvation topology. +- Per-adapter buffer: `adapterInbound := make(chan bridge.Inbound, 32)` (`adapters.go:96`). + `sendInboundWithBackpressure` (`adapters.go:162-177`) blocks indefinitely (ctx only) on + `s.inboundCh`. +- The session-run ledger is process-global (`session-run-exclusivity/spec.md`). Cross-actor + holders (flow steps, cron sentinels, task auto-resume) produce `ErrSessionBusy` that the + single-dispatcher invariant cannot prevent. +- All three adapters support in-place edit: Telegram (`bot.EditMessageText`, + `adapter.go:523`), Slack (`api.UpdateMessageContext`), Mattermost (`client.UpdatePost`). + No emoji-reaction primitive exists on any adapter. The per-session interactive buffer + cap matches `dispatchInboundCap` (both 16, `question.go:43`). +- The TUI drain worker (`internal/app/queue.go:131-212`) treats `ErrSessionBusy` as a + 100 ms retryable backoff. **This precedent does NOT transfer to the bridge without + modification** — see Decision 1 for the critical topology difference. +- `POST /router/inbound` (`http_inbound.go:50-58`) non-blocking select returns bare + `429 "inbound dispatcher full; retry"`. No `Retry-After` header. +- `runNudger` is at `question.go:516-527`; the interactive buffer short-circuit at + `inbound.go:132-138`. + +## Goals / Non-Goals + +**Goals:** +- Human message content is never discarded on `ErrSessionBusy` — TUI parity of intent. +- Cross-actor busy is observable to the user (visibility). +- A single stalled session cannot freeze all other sessions (non-starvation). +- Three silent loss paths become audible. +- Orchestrator-facing 429 is actionable. + +**Non-goals (design level):** +- Message durability across restart — queued message bytes are in-memory; binding rows + persist in `bridge_sessions`. Making shutdown loss log-audible is the requirement; + durable delivery is deferred. +- Unifying the bridge queue with `internal/app/queue.go` — routing bridge inbound through + the app queue would place two queues in series for the same session, making depth + accounting ambiguous. Stated non-goal. +- Adding emoji-reaction primitives to any adapter. + +## Decisions + +### Decision 1: ErrSessionBusy retry — bounded, with non-blocking push to prevent starvation + +**Decision: bounded per-attempt retry (≤ 5 min) + non-blocking push with per-session +overflow to decouple runInboundLoop from per-session channel depth.** + +#### Why unbounded inner-loop retry is unsafe in the bridge topology + +The TUI drain worker (`app/queue.go`) retries `ErrSessionBusy` unboundedly. At first glance +this seems like the right precedent. It is NOT directly applicable: the TUI gives each +session its own goroutine for the entire drain sequence. `runInboundLoop` is a **single +shared goroutine** for all sessions; it calls `dispatchInbound` synchronously; `dispatchInbound` +calls `pushInbound` which blocks when `d.inbound` is full. + +Starvation chain with unbounded in-handle retry: + +1. Session A hits `ErrSessionBusy`. A's per-session `run()` goroutine stays inside + `handleInbound` retrying `agent.Run` — for as long as the competing run (e.g. a flow + step) holds the slot. That can be the entire flow-step duration: `flow-runtime-resume` + spec explicitly says such holders "outlast any budget worth spending". +2. While `run()` is blocked, it does not read from `d.inbound`. New messages for A from + reviewers accumulate in the 16-slot channel. +3. On message 17, `pushInbound` in `dispatchInbound` (called from `runInboundLoop`) blocks + waiting for a slot. +4. `runInboundLoop` is now blocked. Every other session — on every adapter, every identity — + stops being dispatched. One wedged session takes down the whole bridge. + +The TUI's per-session goroutine is isolated; the bridge's shared loop is not. The precedent +does not transfer. + +#### Root-cause fix: non-blocking push with per-session overflow (Option B) + +The cross-session starvation root cause is that `runInboundLoop` can block in `pushInbound` +waiting for a slot on any session's channel. Fix: make `dispatchInbound`'s push to +`d.inbound` non-blocking. If the channel is full, append to a per-session in-memory +`overflow` slice on `sessionDispatch`. `run()` drains the overflow slice after each +`handleInbound` returns, before reading the next message from `d.inbound`, preserving FIFO +ordering. This ensures: + +- `runInboundLoop` completes `dispatchInbound` in O(1) regardless of per-session depth. +- Content is never dropped (overflow is in-memory, unbounded in practice). +- Per-session FIFO is preserved: overflow items are served before new `d.inbound` reads. +- Back-pressure on adapters still propagates through `s.inboundCh` (cap 64) and + `sendInboundWithBackpressure` — the adapter-level stall behavior is unchanged. +- The "NEVER drop (back-pressure adapter instead)" contract is preserved at the adapter + tier; what changes is that the back-pressure no longer reaches the shared loop. + +Considered alternative — goroutine-per-`dispatchInbound` call (Option C): rejected because +two concurrent goroutines pushing for the same session can arrive at `pushInbound` out of +spawn order, breaking per-session FIFO. The overflow slice approach keeps pushes sequential. + +#### ErrSessionBusy retry: bounded at 5 minutes per attempt + +With the non-blocking push fix in place, the per-session `run()` goroutine blocks in +`handleInbound` while retrying — this only stalls that session's own queue (which drains +via overflow), not the shared loop. Unbounded retry is now safe from a system perspective, +but still undesirable: an agent slot held for hours would park one session's queue +indefinitely, with the reviewer seeing increasingly stale "queued" acks. + +Chosen shape: `handleInbound` retries `agent.Run` with 100 ms backoff for up to 5 minutes. +If the budget is exhausted without success, `handleInbound` does NOT discard the message; +instead it signals `run()` to re-queue the item (via a non-blocking push to `d.inbound`, +which goes to overflow if the channel is full). `run()` waits a longer backoff (e.g. 30 s) +before re-reading, giving other queued messages a chance to make progress. The retry clock +resets when the item re-enters the queue. Content is never discarded. + +This asymmetry with the TUI is intentional and documented: the TUI worker can afford +unbounded blocking because it is isolated; the bridge `handleInbound` uses a budget to +allow other pending items for the same session to make progress between reattempts. + +Considered: a shorter budget (1 min). Rejected — cross-actor runs (flow steps) can +legitimately hold a session for several minutes, and a 1-minute budget would re-queue +aggressively, creating unnecessary churn. + +### Decision 2: Where to generate and update the queued-visibility ack + +**Decision: generate in `handleInbound` before the busy-retry inner loop; update inside +the loop on each retry cycle; resolve on loop exit (success or non-busy error).** + +Because `handleInbound` blocks for the run lifetime, it is the natural owner of the ack +lifecycle. The ack token (message ID / ts) returned by `Send` is stored in a local variable +and passed to `Edit` on each retry. Callers do not need a new field on `sessionDispatch` — +this is ephemeral per-inbound-call state. + +The short-wait threshold check (see Decision 3) is also evaluated here: a 2-second timer +arms after the first `ErrSessionBusy`; if the retry loop succeeds before the timer fires, +the ack is never sent. + +### Decision 3: Short-wait threshold for the queued-ack + +**Decision: hardcode 2 seconds for v1; not config-exposed.** + +An ack sent within the same second as the inbound creates a worse UX than no ack: the user +sees "queued" then "processing" nearly simultaneously. The threshold is a UX tuning knob, +not a behavioral invariant, so it need not be in `.opencode.json`. 2 seconds avoids the +flash while remaining short enough that a user waiting 10 seconds definitely sees the ack. + +### Decision 4: In-place edit vs new-message per position update + +**Decision: in-place edit on all three platforms; no new messages posted per position +change.** + +All three adapters already expose edit primitives (Telegram `bot.EditMessageText` +`adapter.go:523`, Slack `api.UpdateMessageContext`, Mattermost `client.UpdatePost`). The +existing `updateAnsweredWidget` in `slack/adapter.go:565` is prior art for in-place update. +Posting a new message per drain step would flood the channel — a session with 5 queued +messages would produce 5 "position updated" posts. Edit failures are logged at info level; +the stale ack is left rather than posting a new one. + +### Decision 5: Shutdown loss — log-only (in-chat not feasible) + +**Decision: mandatory WARN log per dropped message; no in-chat notification.** + +`Service.Stop()` calls `cancel()` first, which cancels the service context. By the time +`tearDownDispatchers()` runs, the service context is done. Any `replyToPeer` call after +that point uses a cancelled context — adapter API calls return immediately on `ctx.Done()` +rather than reaching the platform. Even if adapter goroutines are still alive briefly (the +race window between `cancel()` and goroutine exit), the cancelled context makes any API +call unreliable. + +There is no feasible teardown window in which the adapter is definitely alive and the +context is definitely valid. Attempting advisory in-chat notification would produce +misleading test assertions (passes in timing-lucky runs, silently fails in production). + +The WARN log (with session ID and message count) is the hard requirement. It is testable +against the log sink and meaningful to operators reading server logs. Durable delivery +across restarts remains a named non-goal. + +### Decision 6: Per-adapter ack interface + +**Decision: new optional `QueuedAcknowledger` interface asserted with `ok` pattern; all +three production adapters implement it.** + +Rather than adding methods to the existing `Adapter` interface, `bridge.QueuedAcknowledger` +is asserted per-call — callers check `adapter, ok := a.(bridge.QueuedAcknowledger)` and +skip the ack if the adapter doesn't satisfy it. Test doubles that don't satisfy it behave +as if acks are disabled. + +```go +type QueueAckToken = string // platform-native message ID / ts + +type QueuedAcknowledger interface { + SendQueuedAck(ctx context.Context, peer PeerRef, position int) (QueueAckToken, error) + UpdateQueuedAck(ctx context.Context, peer PeerRef, token QueueAckToken, position int) error +} +``` + +`position` is 1-based (1 = "your message is next"). + +## Risks / Trade-offs + +**Non-blocking push + overflow adds a new in-memory structure per session.** The overflow +slice is unbounded in principle (a very chatty reviewer during a long flow step can +accumulate many entries). In practice the limit is the adapter's own stall behavior: +`sendInboundWithBackpressure` stalls the adapter pump when `s.inboundCh` (cap 64) is full, +so at most 64 unprocessed items can accumulate system-wide before adapters back-pressure. +Per-session overflow depth is bounded by that shared cap. Risk is low. + +**Re-queue on `ErrSessionBusy` makes `TestRunFailureMessage_BusyDoesNotAdviseAbort` wrong.** +The test pins lossy wording that will no longer be reachable. It must be deliberately +updated; the tasks call this out explicitly. + +**5-minute per-attempt budget means a persistently busy session re-queues every 5 min.** +If the competing run is a 2-hour flow step, the message re-queues ~24 times, each time +resetting the 2-second ack threshold timer. The user sees the ack update on each cycle. +This is acceptable behavior: the reviewer knows their message is alive and being retried. + +**Teardown WARN log requires draining `d.inbound` before `close()`.** The current +`tearDownDispatchers` closes without draining (`dispatch.go:897-902`). The implementation +must drain first (collect to a slice, log, then close). This is a behavior change to +`sessionDispatch.close()` that must not introduce a race with the `run()` goroutine. diff --git a/openspec/changes/bridge-queue-visibility-and-loss-paths/proposal.md b/openspec/changes/bridge-queue-visibility-and-loss-paths/proposal.md new file mode 100644 index 0000000000..8b26d30cd5 --- /dev/null +++ b/openspec/changes/bridge-queue-visibility-and-loss-paths/proposal.md @@ -0,0 +1,134 @@ +## Why + +The bridge already queues inbound messages — `sessionDispatch.inbound` is a buffered channel +(cap 16) that back-pressures adapters rather than dropping — but from a user's perspective +the queue is completely invisible: no acknowledgement is sent when a message lands behind an +in-flight run. Worse, the current `ErrSessionBusy` handler in `handleInbound` is both +incorrect and lossy: when a competing actor (flow step, cron sentinel, task auto-resume) +holds the session slot, `agent.Run` returns `ErrSessionBusy`, the inbound message is silently +discarded, and the user is told to "please resend". The `chat-bridge` spec's justification +for asserting `ErrSessionBusy` can never surface — "all reviewer fan-in goes through this +single dispatcher" — is factually wrong; it only rules out bridge-vs-bridge collisions, not +cross-actor holders, which `session-run-exclusivity/spec.md:12-14` explicitly calls out by +name. Three additional loss paths produce no user-facing signal at all: the interactive-flow +buffer's drop-oldest truncation, the early-exit when no active agent is available, and the +bare `429` from `POST /router/inbound`. + +## What Changes + +- **`chat-bridge` requirement R7 is amended (spec correction).** The "MUST NEVER return + `ErrSessionBusy`" assertion is removed; the requirement now specifies the correct + re-queue-and-retry semantics for cross-actor busy responses. The associated test + `TestRunFailureMessage_BusyDoesNotAdviseAbort` currently pins the lossy wording and must + be deliberately updated as part of this change. + +- **`ErrSessionBusy` from cross-actor holders is handled correctly.** When `agent.Run` + returns `ErrSessionBusy` because a flow-step agent, a cron sentinel lock, or a + task-auto-resume holds the session slot, the bridge MUST retain and retry the inbound + with a bounded per-attempt budget (≤ 5 min); on budget exhaustion the message is + re-queued via the overflow mechanism for a subsequent attempt. Human message content + MUST NEVER be discarded on `ErrSessionBusy`, regardless of how long the competing run + lasts. The TUI drain worker's unbounded retry precedent does NOT directly apply — the + bridge funnels all sessions through a single shared dispatch loop whose blocking + semantics require the non-blocking push fix below. + +- **Non-blocking push with per-session overflow (fixes cross-session starvation).** + `runInboundLoop` (`service.go:426`) is a single shared goroutine; `dispatchInbound` + currently calls `pushInbound` which blocks when `d.inbound` is full, stalling ALL other + sessions. This change makes `dispatchInbound`'s push to `d.inbound` non-blocking: + overflow messages go to a per-session in-memory slice drained by `run()` after each + `handleInbound`, preserving FIFO. A single stalled session MUST NOT prevent dispatch to + any other session. + +- **Queued-acknowledgement visibility (new capability, new config field).** When an inbound + message queues behind a run already in flight, the sender SHALL receive an acknowledgement + naming its queue position. The acknowledgement is updated in-place (edit, not post) as the + queue drains and resolved when the message begins its run. Config-gated under + `router.queueAcknowledgementsEnabled` following the `router.toolUpdatesEnabled` precedent. + This is a new `Router.*` config field and therefore triggers all four schema-update + obligations from CLAUDE.md. + +- **Interactive-flow buffer drops are audible.** When `QuestionRouter.BufferInbound` evicts + an oldest message (cap 16), the evicted sender SHALL be notified via a reply rather than + losing their message silently. + +- **No-active-agent drop is audible.** When `handleInbound` exits early because + `ActiveAgent()` is nil, the inbound sender SHALL be informed rather than seeing nothing. + +- **`POST /router/inbound` 429 gains `Retry-After` and depth.** The bare "retry" text + response is enriched with a `Retry-After` header and a machine-readable dispatcher + saturation body so the orchestrator's retry is informed rather than blind. + +- **Shutdown loss is log-audible.** Messages queued in a dispatcher at `Service.Stop` time + are drained for logging (WARN per message, per-session count summary) before the channel + is closed. In-chat notification at teardown is not attempted: `Service.Stop()` cancels + the service context before `tearDownDispatchers()`; any `replyToPeer` call using that + cancelled context returns immediately without reaching the platform. Durable delivery + across restarts is an explicit non-goal. + +### Non-goals + +- **No queue for the non-bridge HTTP message API.** `POST /session/{id}/message` and + `POST /session/{id}/prompt_async` (`internal/api/handler_message.go`) return 409 Conflict + today; queueing a synchronous HTTP request means holding the connection open for an + unbounded wait, which is a different contract question, explicitly out of scope. +- **No unification of the three inbound queues** (bridge `sessionDispatch.inbound`, + `internal/app/queue.go`, any future HTTP one). Routing bridge inbound through the app + queue would place two queues in series for the same session, making depth accounting and + ordering ambiguous. Explicitly deferred. +- **Flow-owned / non-interactive runs unchanged.** The orchestrator drives opencode via + `POST /flow` with `NonInteractive: true`; the flow engine owns the prompt sequence there, + so a human-style queue is inapplicable. +- **No emoji-reaction primitives.** No adapter gains a reaction API in this change; the + queued-ack uses editable text notes on all three platforms. +- **No ACP changes.** ACP is serial by construction over stdio. + +## Capabilities + +### New Capabilities + +- `bridge-queue-visibility`: the queued-acknowledgement contract — when to send it (position + ≥ 1), in-place-edit semantics, position reporting, the short-wait threshold before + announcing, config gating, group/DM routing (sender only), and lifecycle (resolved when the + message begins its run; never lingered after the run starts). + +### Modified Capabilities + +- `chat-bridge`: amend the per-session dispatcher requirement to replace the incorrect + "MUST NEVER return `ErrSessionBusy`" assertion with the correct re-queue-and-retry + contract; add requirements for the three silent-drop paths (interactive-flow buffer + overflow, no-active-agent, shutdown loss). +- `bridge-http-api`: add `Retry-After` header and machine-readable saturation body to + `POST /router/inbound` 429 responses. + +## Impact + +**`github.com/opencode-ai/opencode`** + +- `internal/bridge/service/dispatch.go:193-196` (`handleInbound`, no-agent branch): reply + to sender instead of silently returning. +- `internal/bridge/service/dispatch.go:218-223` (`handleInbound`, `agent.Run` error path): + on `ErrSessionBusy`, retry with bounded budget; on exhaustion signal `run()` to re-queue + via overflow rather than discarding. `ErrSessionBusy` no longer reaches `runFailureMessage`. +- `internal/bridge/service/dispatch.go:258-269` (`runFailureMessage`): the `ErrSessionBusy` + arm becomes dead code and is removed. +- `internal/bridge/service/inbound.go:dispatchInbound` + `internal/bridge/service/dispatch.go` + (`sessionDispatch`, `pushInbound`): replace blocking `pushInbound` call from the shared + loop with a non-blocking push; add per-session `overflow []bridge.Inbound` slice; add + `run()` overflow-drain step after each `handleInbound`. +- `internal/bridge/service/dispatch_test.go:60-74` (`TestRunFailureMessage_BusyDoesNotAdviseAbort`): + deliberately updated — the test pins the old lossy wording and must change. +- `internal/bridge/service/question.go:472-481` (`BufferInbound`): notify the dropped + sender when cap is hit. +- `internal/bridge/service/http_inbound.go:50-58` (`handleInbound` HTTP handler, 429 path): + add `Retry-After` header and structured saturation body. +- `internal/config/config.go`: new `QueueAcknowledgementsEnabled bool` field under + `RouterConfig` (or equivalent; naming follows existing `ToolUpdatesEnabled`). +- `cmd/schema/main.go` + `opencode-schema.json`: schema update and regeneration for the new + field (CLAUDE.md obligation: non-negotiable). +- `docs/bridge.md`: document the new field and queue-acknowledgement behavior. +- `internal/config/config_test.go` (or new `config_router_test.go`): Viper round-trip unit + test for the new `RouterConfig` field (CLAUDE.md obligation). +- All three platform adapters (`telegram/`, `slack/`, `mattermost/`): new + `SendQueuedAck(ctx, peer, pos) (editToken, error)` and + `UpdateQueuedAck(ctx, peer, editToken, pos)` plumbing, or equivalent interface. diff --git a/openspec/changes/bridge-queue-visibility-and-loss-paths/specs/bridge-http-api/spec.md b/openspec/changes/bridge-queue-visibility-and-loss-paths/specs/bridge-http-api/spec.md new file mode 100644 index 0000000000..8354cf9074 --- /dev/null +++ b/openspec/changes/bridge-queue-visibility-and-loss-paths/specs/bridge-http-api/spec.md @@ -0,0 +1,53 @@ +## Purpose + +Delta spec for the `bridge-http-api` capability. Amends the `POST /router/inbound` 429 +response to carry a `Retry-After` header and machine-readable saturation body so an +orchestrator mediator can back off intelligently rather than guessing. The existing +single-retry policy of the c2-agent orchestrator makes a blind 429 a real message-loss +risk when the channel is transiently saturated. + +## MODIFIED Requirements + +### Requirement: POST /router/inbound 429 carries Retry-After and saturation body + +When `POST /router/inbound` returns `429 Too Many Requests` because the shared inbound +channel is full (`default:` branch in the non-blocking select), the response SHALL include: + +1. A `Retry-After` header with an integer value (seconds) indicating the minimum wait + before retrying. The value SHALL be derived from the expected drain rate; a value of + `1` (one second) is a safe conservative default for v1. +2. A JSON response body of the form: + ```json + { + "error": "inbound dispatcher full", + "retryAfterSeconds": , + "dispatcherSaturated": true + } + ``` + The `dispatcherSaturated: true` field is a stable machine-readable signal that allows + mediators to distinguish a capacity-related 429 from a rate-limiting 429 that might + originate from other middleware. + +The existing `"inbound dispatcher full; retry"` string is replaced by the structured body +above. Callers that parse only the status code are unaffected (they still receive 429). + +#### Scenario: Channel full returns enriched 429 + +- **GIVEN** the shared `inboundCh` (cap 64) is full when `POST /router/inbound` arrives +- **WHEN** the non-blocking select takes the `default:` branch +- **THEN** the response is `429 Too Many Requests` with: + - `Retry-After: 1` (or the computed value) in the response header + - JSON body `{"error":"inbound dispatcher full","retryAfterSeconds":1,"dispatcherSaturated":true}` + +#### Scenario: Normal enqueue returns 202 Accepted unchanged + +- **GIVEN** the shared `inboundCh` has capacity +- **WHEN** `POST /router/inbound` arrives with a valid body +- **THEN** the response is `202 Accepted` with `{"ok":true}`; no behavior change + +#### Scenario: Mediator observes dispatcherSaturated to distinguish from rate-limit + +- **GIVEN** an orchestrator mediator receives a 429 from `POST /router/inbound` +- **WHEN** the mediator inspects `dispatcherSaturated` in the response body +- **THEN** `true` indicates a capacity constraint (retry after the header-specified delay); + absent or `false` would indicate a different 429 origin (distinguishing future cases) diff --git a/openspec/changes/bridge-queue-visibility-and-loss-paths/specs/bridge-queue-visibility/spec.md b/openspec/changes/bridge-queue-visibility-and-loss-paths/specs/bridge-queue-visibility/spec.md new file mode 100644 index 0000000000..2ad9986e85 --- /dev/null +++ b/openspec/changes/bridge-queue-visibility-and-loss-paths/specs/bridge-queue-visibility/spec.md @@ -0,0 +1,141 @@ +## Purpose + +Provides visible acknowledgement to a chat-bridge user whose inbound message has been +accepted into the dispatcher queue but cannot start its agent run immediately because a +run is already in flight for that session. The acknowledgement tells the sender their +message is queued and will be delivered; it is updated in-place as the queue drains and +resolved the moment the message begins its run, so no stale "queued" note ever lingers. + +## ADDED Requirements + +### Requirement: Queued-acknowledgement on per-session inbound backlog + +When an inbound message is enqueued on a `sessionDispatch.inbound` channel while a run is +already in flight for that session, the bridge SHALL send a queued-acknowledgement reply to +the message's sender if `cfg.Router.QueueAcknowledgementsEnabled` is true. The +acknowledgement SHALL identify that the message is queued and, when multiple messages are +queued, its ordinal position (e.g. "queued — 2 messages ahead"). + +This requirement covers only messages that reach the dispatcher's inbound channel while a +run is in flight. Messages that enter the channel while no run is active start immediately +and produce no acknowledgement. + +#### Scenario: First message queues behind an in-flight run + +- **GIVEN** a run is in flight for session S and `QueueAcknowledgementsEnabled` is true +- **WHEN** an inbound message M arrives from peer P and is pushed to the channel +- **THEN** the bridge sends an acknowledgement to peer P indicating M is queued (e.g. + "⏳ Your message is queued — 1 ahead. I'll respond once the current run finishes.") + +#### Scenario: Second queued message shows its position + +- **GIVEN** a run is in flight for session S and one message is already queued +- **WHEN** a second inbound message arrives from peer P +- **THEN** the acknowledgement for the second message indicates 2 messages are ahead + +#### Scenario: Acknowledgements disabled + +- **GIVEN** `QueueAcknowledgementsEnabled` is false (or unset) +- **WHEN** an inbound message queues behind an in-flight run +- **THEN** no acknowledgement is sent; the message is silently queued as before + +### Requirement: Acknowledgements are updated in-place as the queue drains + +The bridge SHALL update each sender's queued-acknowledgement in-place (using the +platform's edit-message primitive) rather than posting a new message per state change, on +all platforms that support edit (Telegram, Slack, and Mattermost all do). The in-place +update SHALL reflect the current queue position as earlier messages begin their runs and +the sender's message moves up. + +No emoji-reaction primitives are used. The acknowledgement is a text message that is +edited; no reaction-based design (👀 / ✅) is employed or required. + +#### Scenario: Queue drains; position updates in-place + +- **GIVEN** peer P's message is queued at position 3 and their acknowledgement was sent +- **WHEN** two earlier messages complete their runs (queue drains by 2) +- **THEN** the bridge edits the existing acknowledgement message for P to indicate position 1, + rather than posting two new messages + +#### Scenario: Edit fails gracefully + +- **GIVEN** the adapter returns an error when editing the acknowledgement (e.g. message too + old, scope missing) +- **THEN** the bridge logs the failure at info level and does not retry the edit; the + original acknowledgement remains visible to the sender with stale position information; + no new message is posted + +### Requirement: Acknowledgement is resolved when the message begins its run + +When the queued message is dequeued and its agent run begins, the bridge SHALL resolve the +acknowledgement by editing it to a "running" state (e.g. "▶ Processing your message now…") +or deleting it. The bridge MUST NOT leave a "queued" acknowledgement visible after the run +has started. + +#### Scenario: Run starts; acknowledgement resolved + +- **GIVEN** peer P's message was queued and the acknowledgement was sent +- **WHEN** the dispatcher dequeues P's message and `agent.Run` is called +- **THEN** before blocking on the run, the bridge edits the acknowledgement to indicate the + run has started (or deletes it); the "queued" state is no longer visible to peer P + +#### Scenario: Run fails to start (non-busy error); acknowledgement resolved + +- **GIVEN** peer P's message was queued and the acknowledgement was sent +- **WHEN** `agent.Run` returns an error other than `ErrSessionBusy` +- **THEN** the bridge edits or deletes the queued acknowledgement and sends the normal + run-failure reply; no stale "queued" notice lingers + +### Requirement: Short-wait threshold before announcing + +The bridge SHALL NOT send a queued-acknowledgement when the queue depth at the time of +enqueueing is zero (i.e. the message is the only one queued) AND the run that is blocking +it has been running for less than a configurable short-wait threshold (default 2 seconds). +This avoids a pointless "queued" flash for inbound messages that start within human +reaction time. If the run is still in flight after the threshold, the bridge sends the +acknowledgement. + +The threshold is an internal implementation decision for v1 — it MAY be hardcoded at 2 +seconds and need not be config-exposed. + +#### Scenario: Message queues for a sub-threshold wait + +- **GIVEN** a run has been in flight for 0.5 seconds and one inbound queues +- **WHEN** the run completes within the short-wait threshold +- **THEN** no acknowledgement was ever sent to the sender + +#### Scenario: Message queues for a long wait (threshold exceeded) + +- **GIVEN** a run has been in flight for 30 seconds when an inbound queues +- **THEN** the acknowledgement is sent immediately (threshold already exceeded) + +### Requirement: Acknowledgement is sent to the sender only in group/multi-peer sessions + +In sessions bound to multiple peers (group channels or multi-reviewer flows), the queued +acknowledgement SHALL be sent to the specific peer whose inbound was queued, not +broadcast to all bound peers. Other peers' conversations MUST NOT be cluttered with +acknowledgements for messages they did not send. + +#### Scenario: Multi-peer session; ack goes to sender only + +- **GIVEN** session S is bound to Alice (Slack) and Bob (Telegram) and a run is in flight +- **WHEN** Alice sends a message that queues +- **THEN** Alice receives the acknowledgement in her DM; Bob's conversation is unchanged + +### Requirement: Queue-acknowledgement feature is config-gated + +The queued-acknowledgement behavior SHALL be controlled by a boolean field +`QueueAcknowledgementsEnabled` on the bridge's `RouterConfig` struct (or equivalent name +consistent with the existing `ToolUpdatesEnabled` precedent). When false or absent, the +bridge behaves as before this change — no acknowledgements are sent, messages queue +silently. + +Adding this field to `RouterConfig` triggers all four CLAUDE.md schema obligations: +(1) update `cmd/schema/main.go`, (2) regenerate `opencode-schema.json`, (3) update +`docs/bridge.md`, (4) add a Viper round-trip unit test in `internal/config/`. + +#### Scenario: Field absent in .opencode.json + +- **GIVEN** `.opencode.json` contains a `router` section without `queueAcknowledgementsEnabled` +- **THEN** the field defaults to false; no acknowledgements are sent; Viper's case-fold + does not mangle the absent key diff --git a/openspec/changes/bridge-queue-visibility-and-loss-paths/specs/chat-bridge/spec.md b/openspec/changes/bridge-queue-visibility-and-loss-paths/specs/chat-bridge/spec.md new file mode 100644 index 0000000000..abba3d97c3 --- /dev/null +++ b/openspec/changes/bridge-queue-visibility-and-loss-paths/specs/chat-bridge/spec.md @@ -0,0 +1,178 @@ +## Purpose + +Delta spec for the `chat-bridge` capability. Amends the per-session dispatcher requirement +to (a) remove the incorrect "MUST NEVER return `ErrSessionBusy`" assertion, (b) specify the +correct content-preserving retry contract, (c) add the cross-session non-starvation +requirement, and (d) add three missing requirements for previously silent loss paths: +interactive-flow buffer overflow, no-active-agent drop, and in-flight message loss on +process shutdown. + +## MODIFIED Requirements + +### Requirement: Inbound dispatch MUST NOT starve other sessions (cross-session non-starvation) + +The bridge's shared `runInboundLoop` (`service.go:426-438`) processes all sessions through +a single goroutine, calling `dispatchInbound` synchronously for each message. To prevent +one stalled session from blocking dispatch to all others, `dispatchInbound`'s push to any +per-session `d.inbound` channel MUST be non-blocking from the shared loop's perspective. +When a session's `d.inbound` channel is full, the message SHALL be appended to a per-session +in-memory overflow slice on `sessionDispatch` rather than blocking `runInboundLoop`. The +per-session `run()` goroutine SHALL drain the overflow slice after each `handleInbound` call +completes, before reading the next message from `d.inbound`, preserving per-session FIFO. + +This non-starvation requirement is independent of `ErrSessionBusy` handling — it applies to +the push path regardless of why a session's channel is full. "NEVER drop (back-pressure +adapter instead)" refers to back-pressuring the per-adapter pump goroutine at the +`s.inboundCh` (cap 64) level; it never meant the shared loop should block per-session. + +#### Scenario: Session A full; session B dispatched without delay + +- **GIVEN** session A's `d.inbound` channel is full (a long-running agent turn is in flight) +- **WHEN** messages arrive for both session A and session B via `runInboundLoop` +- **THEN** session B's message is dispatched immediately; session A's message is stored in + A's overflow slice and delivered after A's current turn completes; neither message is + dropped; `runInboundLoop` is NOT blocked + +#### Scenario: Overflow slice drains in FIFO order + +- **GIVEN** session A has 3 messages in its overflow slice (accumulated while channel was + full) and A's current run just completed +- **WHEN** `run()` exits `handleInbound` and checks the overflow slice +- **THEN** the 3 overflow messages are transferred to `d.inbound` in arrival order; + subsequent reads from `d.inbound` deliver them before any newly arriving messages + +### Requirement: Per-`sessionId` dispatch goroutine with dual-channel select (amended) + +For each actively-bound `sessionId` the bridge SHALL run exactly **one** dispatcher goroutine +that owns both inbound message dispatch and parts demultiplexing for that session. The +dispatcher MUST use a single `select{}` over two channels: + +| Channel | Capacity | Drop policy | Source | +|---|---|---|---| +| inbound | 16 | NEVER drop — overflow to per-session slice when full (non-starvation requirement); back-pressure propagates at `s.inboundCh` level | per-peer adapter goroutines via runInboundLoop | +| parts | 64 | drop-oldest with rate-limited log | broker-receive goroutine non-blocking forward | + +The dispatcher MUST call `agent.Run` serially — only one in-flight Run per session at a +time. It MUST consume the Run channel's terminal `AgentEvent` before processing the next +inbound message. + +**`ErrSessionBusy` from cross-actor holders is a legitimate outcome and MUST be handled by +content-preserving retry, not by discarding.** The session-run ledger is process-global +(`session-run-exclusivity` spec): any holder — a flow step's own agent instance, a cron +sentinel lock, a task auto-resume — makes `agent.Run` return `ErrSessionBusy`. The +single-dispatcher serialization only prevents bridge-vs-bridge collisions; it cannot prevent +cross-actor collisions. When `agent.Run` returns `ErrSessionBusy`, the bridge MUST: + +1. Retain the inbound message content — it MUST NOT be discarded. +2. Retry with a short backoff (≤ 200 ms per attempt) for a bounded per-attempt budget. +3. On budget exhaustion, re-queue the message for a later attempt rather than discarding. + Content preservation is unconditional and budget-independent. +4. Inform the sender (via `bridge-queue-visibility` if enabled) that their message is + waiting behind a run it does not own. + +The per-attempt budget is intentionally distinct from the TUI drain worker's unbounded +retry: the TUI uses a per-session goroutine isolated from other sessions; the bridge +`handleInbound` uses a bounded budget so other items queued behind it can make progress +between reattempts, with the message re-entering the queue rather than being discarded. + +The dispatcher's lifecycle is tied to the binding: created on first `Bind(sessionId, ...)`, +torn down on `Unbind(sessionId)` or when the bridge observes `session_id == NULL`. + +#### Scenario: Cross-actor `ErrSessionBusy` is retried, not discarded + +- **GIVEN** a flow step's agent instance holds the session slot for session S +- **WHEN** an inbound message M from peer P reaches the dispatcher and `agent.Run` returns + `ErrSessionBusy` +- **THEN** M is retained and retried with backoff; M is NOT discarded and the user is NOT + told to resend; if the per-attempt budget expires, M is re-queued for a subsequent attempt + via the overflow mechanism, never dropped + +#### Scenario: Budget expires; message re-queued, not discarded + +- **GIVEN** a flow step holds session S's slot for longer than the per-attempt retry budget +- **WHEN** the budget expires without the slot freeing +- **THEN** M's content is preserved and re-queued (via the overflow slice or equivalent); + the session's dispatcher may process other pending messages before retrying M; + M is eventually delivered when the slot frees; the sender's queued-ack is updated + +#### Scenario: Retry succeeds when the competing run finishes + +- **GIVEN** M was re-queued after an `ErrSessionBusy` from a flow-step holder +- **WHEN** the flow step's run completes and releases the session slot +- **THEN** the dispatcher's next attempt of `agent.Run` succeeds; M's full content is + delivered to the agent as if it had arrived after the competing run + +#### Scenario: Bridge-originated `ErrSessionBusy` cannot occur + +- **WHEN** Alice and Bob both send messages to session S within milliseconds of each other +- **THEN** their messages land on the per-session inbound channel in arrival order; the + dispatcher processes Alice's full agent turn before pulling Bob's message; no + `ErrSessionBusy` ever surfaces from bridge-internal serialization + +#### Scenario: Parts overflow drops oldest, logs once per session per minute + +- **WHEN** a session emits more than 64 part events while its sender is blocked on + outbound IO +- **THEN** the oldest part is dropped, the newest appended, and a warn-level overflow log + is emitted (rate-limited to once per session per minute) + +### Requirement: No-active-agent drop is audible + +When `handleInbound` is reached but `ActiveAgent()` returns nil — meaning no agent is +registered in the process — the bridge MUST NOT silently discard the inbound. The bridge +SHALL reply to the sender's peer with a brief error explaining that no agent is available, +so the sender knows their message was not processed and can retry or escalate. + +#### Scenario: Inbound arrives when no agent is configured + +- **GIVEN** `app.ActiveAgent()` is nil (e.g. the agent service has not initialized or + has been torn down) +- **WHEN** an inbound message arrives from peer P for session S +- **THEN** the bridge sends a reply to P explaining the message could not be processed + due to no active agent; the message is not delivered silently to /dev/null + +### Requirement: Interactive-flow buffer overflow is audible + +When `QuestionRouter.BufferInbound` evicts the oldest buffered message because the +per-session interactive buffer is full (`interactiveInboundBufferCap`, currently 16), the +bridge MUST reply to the peer whose message was dropped informing them that their input was +not retained and they should resend it after the current interactive step concludes. A +warn-level log entry remains required; the user-visible reply is additive. + +#### Scenario: Interactive-buffer overflow evicts message 17 + +- **GIVEN** session S is in an interactive flow step with 16 messages already buffered and + no question pending +- **WHEN** a 17th inbound message arrives from peer P +- **THEN** the oldest buffered message (message 1) is evicted; the bridge sends a reply to + that message's peer notifying them that their input was dropped and should be resent; the + warn-level log is also emitted + +### Requirement: Queued messages logged at WARN on shutdown; in-chat notification not required + +When `Service.Stop` tears down a `sessionDispatch` that has messages queued in its inbound +channel or per-session overflow slice, the bridge SHALL drain those messages BEFORE closing +the channel and SHALL emit one WARN log entry per dropped message (session ID, peer ID) plus +a per-session summary count. + +In-chat notification to senders at shutdown is NOT required and MUST NOT be specced as even +advisory. `Service.Stop()` cancels the service context before `tearDownDispatchers()` runs; +any adapter API call using that cancelled context returns immediately without reaching the +platform. Speccing unreliable behavior produces tests that pass in timing-lucky runs and +fail silently in production. + +Durability across process restarts is explicitly out of scope for this change. + +#### Scenario: Shutdown with queued messages + +- **GIVEN** session S's dispatcher has 3 messages in its inbound channel when + `Service.Stop` is called +- **THEN** before closing `d.inbound`, the bridge drains the channel; for each item it logs + at WARN: `"bridge: shutdown lost queued inbound session= peer=

"`; a final WARN + carries the total count; no in-chat reply is sent or attempted + +#### Scenario: Clean shutdown with empty queues + +- **GIVEN** all session dispatchers have empty inbound channels and overflow slices at + shutdown time +- **THEN** no shutdown-loss warn is emitted; shutdown is silent as before diff --git a/openspec/changes/bridge-queue-visibility-and-loss-paths/tasks.md b/openspec/changes/bridge-queue-visibility-and-loss-paths/tasks.md new file mode 100644 index 0000000000..22b0f1d778 --- /dev/null +++ b/openspec/changes/bridge-queue-visibility-and-loss-paths/tasks.md @@ -0,0 +1,228 @@ +# Tasks: bridge-queue-visibility-and-loss-paths + +## 1. Fix `ErrSessionBusy` — re-queue instead of discard + +- [x] 1.1 In `internal/bridge/service/dispatch.go`'s `handleInbound`, replace the + discard-and-reply path for `ErrSessionBusy` with a bounded retry loop (budget: 5 minutes, + backoff: 100 ms per cycle, matching `internal/app/queue.go`'s `busyBackoff`). On each + retry, re-call `ag.Run` with the original `in` value. When the budget expires WITHOUT a + successful `ag.Run`, signal `run()` to re-queue `in` via the overflow slice (task 1.6) + rather than discarding. The `in` value MUST never be lost due to a busy session. + +- [x] 1.2 Remove the `ErrSessionBusy` arm from `runFailureMessage` + (`dispatch.go:258-269`). The arm is now dead code. If `runFailureMessage` is still + needed for non-busy errors, retain the generic branch only; delete the specific busy text. + +- [x] 1.3 Update `TestRunFailureMessage_BusyDoesNotAdviseAbort` + (`internal/bridge/service/dispatch_test.go:60-74`). This test currently pins the lossy + wording ("resend"). After 1.1-1.2 the `ErrSessionBusy` case no longer reaches + `runFailureMessage`. Update the test to assert the re-queue / retry behavior: verify that + when `agent.Run` returns `ErrSessionBusy` once and then succeeds, the inbound content + is delivered (not discarded). Do NOT simply delete the test — convert it to cover the + retry invariant. + +- [x] 1.4 Add a new test `TestHandleInbound_BusyRetryPreservesContent` in + `dispatch_test.go` or a new `dispatch_busy_retry_test.go`: given a mock agent that + returns `ErrSessionBusy` N times then succeeds, assert (a) `agent.Run` is called N+1 + times with the same content, (b) the run-failure reply path is NOT taken, + (c) the inbound text is intact when the final Run call succeeds. + +- [x] 1.6 Implement non-blocking push with per-session overflow to fix cross-session + starvation: + + a. Add `overflow []bridge.Inbound` slice and protecting mutex to `sessionDispatch` + (`dispatch.go`). + + b. In `dispatchInbound` (`inbound.go`), replace the blocking `disp.pushInbound(ctx, in)` + call with a non-blocking attempt: try a non-blocking send to `d.inbound`; if the + channel is full, append to `d.overflow` instead. `runInboundLoop` MUST NOT block + waiting for any per-session slot. + + c. In `run()` (`dispatch.go:124-138`), after `d.handleInbound` returns, drain + `d.overflow` into `d.inbound` (FIFO) before the next iteration reads a new message + from `d.inbound`. Overflow items are served before new channel reads. + + d. In `sessionDispatch.close()` (`dispatch.go:897-902`), drain both `d.overflow` and + the unread items in `d.inbound` for shutdown-loss logging (task 4.1) BEFORE calling + `close(d.inbound)`. + + e. Add a test `TestDispatch_NonBlockingPush_NoStarvation`: construct two session + dispatchers; fill session A's `d.inbound` to cap; push one message for session A and + one for session B via the push path; assert session B's message is accepted + immediately (no block) and that session A's overflow slice holds the message. + +- [x] 1.7 Add a test `TestSessionSerializationInvariant` that verifies the dispatcher + processes a second inbound message only after the first `agent.Run` returns: use a + slow mock that returns after a controlled delay; assert delivery order is strictly FIFO + and no concurrent Runs are observed. This pins the serialization behavior currently + untested. + +## 2. No-active-agent drop: make it audible + +- [x] 2.1 In `handleInbound` (`dispatch.go:193-196`), after logging the warn, call + `d.svc.replyToPeer` (passing `in.Peer`) with a brief message such as "bridge: this + session has no active agent — your message could not be processed. Please try again + once the agent is available." Do NOT retry in this branch; the condition requires + operator intervention. + +- [x] 2.2 Add a test `TestHandleInbound_NilAgentReplies`: a dispatcher with a nil + `ActiveAgent()` receives an inbound; assert a reply is sent to the sender and no panic + occurs. + +## 3. Interactive-flow buffer overflow: make it audible + +- [x] 3.1 In `QuestionRouter.BufferInbound` (`question.go:472-481`): when the buffer is + full and the oldest message is evicted, capture `evicted.Peer` and call + `r.svc.replyToPeer` with a message such as "bridge: your earlier message was lost + because too many messages were buffered while the interactive step had no pending + question. Please resend it once the current step completes." + +- [x] 3.2 Add a test `TestBufferInbound_DropNotifiesEvictedPeer`: fill the buffer to cap, + push one more; assert the evicted peer receives a reply and the buffer still has cap + elements with the newest message at the tail. + +## 4. Shutdown loss: log at WARN + +- [x] 4.1 In `sessionDispatch.close()` (`dispatch.go:897-902`), BEFORE calling + `close(d.inbound)`, drain the channel into a local slice. Also drain `d.overflow`. + For each item collected, log at WARN: + `"bridge: shutdown lost queued inbound", "session", d.sessionID, "peer", item.Peer.PeerID`. + Emit a per-session summary: `"bridge: shutdown dropped N queued messages", "session", d.sessionID`. + Draining must be protected against concurrent `run()` reads — acquire the overflow mutex + before draining, and only drain `d.inbound` items that remain after `d.stop.Store(true)` + (i.e., items the `run()` goroutine can no longer read). + +- [x] 4.2 Add a test `TestShutdown_WarnsOnQueuedMessages`: construct a dispatcher with N + messages in its inbound channel; call `close()`; assert N WARN log entries containing + the session ID and peer ID are emitted. No assertion on in-chat replies (they are not + attempted). + +## 5. POST /router/inbound 429 enrichment + +- [x] 5.1 In `internal/bridge/service/http_inbound.go`'s `handleInbound` HTTP handler, + replace the bare `writeAPIError(w, http.StatusTooManyRequests, "inbound dispatcher full; retry")` + with: + ```go + w.Header().Set("Retry-After", "1") + writeJSON(w, http.StatusTooManyRequests, map[string]any{ + "error": "inbound dispatcher full", + "retryAfterSeconds": 1, + "dispatcherSaturated": true, + }) + ``` + +- [x] 5.2 Update the existing 429 test in `internal/bridge/service/http_inbound_test.go` + to assert: (a) `Retry-After: 1` header is present, (b) the response body contains + `dispatcherSaturated: true`, (c) the HTTP status is still 429. + +## 6. Config field: `QueueAcknowledgementsEnabled` + +- [x] 6.1 Add `QueueAcknowledgementsEnabled bool` to the `RouterConfig` struct (or + whatever the existing bridge router config struct is named) in + `internal/config/config.go`. Add a field comment explaining the behavior and noting + that `false` (the default) disables all queued acknowledgements. + +- [x] 6.2 Update `cmd/schema/main.go` to declare the new field with `type: boolean`, + description, and `default: false`. Regenerate `opencode-schema.json` via + `go run cmd/schema/main.go > opencode-schema.json` and commit the result. (**CLAUDE.md + schema obligation — non-negotiable.**) + +- [x] 6.3 Update `docs/bridge.md` with a `router.queueAcknowledgementsEnabled` entry in + the config reference section, explaining what it does and its default. (**CLAUDE.md + docs obligation.**) + +- [x] 6.4 Add a Viper round-trip unit test in `internal/config/` (e.g. + `config_router_test.go`): unmarshal `.opencode.json` with `router.queueAcknowledgementsEnabled: true` + via `viper.Unmarshal`, assert the field is `true` in the resulting struct. Pure + `json.Unmarshal` is insufficient — Viper case-folds map keys and the issue manifests + only through the real loader path. (**CLAUDE.md Viper round-trip obligation.**) + +## 7. `QueuedAcknowledger` interface and adapter implementations + +- [x] 7.1 Define the `QueuedAcknowledger` interface in `internal/bridge/bridge.go` (or + the existing package-level types file): + ```go + type QueueAckToken = string + + type QueuedAcknowledger interface { + SendQueuedAck(ctx context.Context, peer PeerRef, position int) (QueueAckToken, error) + UpdateQueuedAck(ctx context.Context, peer PeerRef, token QueueAckToken, position int) error + } + ``` + `position` is 1-based. Assert the interface via compile-time check on each adapter. + +- [x] 7.2 Implement `SendQueuedAck` and `UpdateQueuedAck` for the Telegram adapter + (`internal/bridge/telegram/adapter.go`): `SendQueuedAck` calls `bot.SendMessage` with + the queued-ack text; `UpdateQueuedAck` calls `bot.EditMessageText` with the token + (message ID cast to int). Message IDs on Telegram are ints; the token is the + string-encoded ID. + +- [x] 7.3 Implement for the Slack adapter (`internal/bridge/slack/adapter.go`): + `SendQueuedAck` calls `api.PostMessageContext`; `UpdateQueuedAck` calls + `api.UpdateMessageContext`. The token is the message `ts` string. + +- [x] 7.4 Implement for the Mattermost adapter (`internal/bridge/mattermost/adapter.go`): + `SendQueuedAck` calls `client.CreatePost`; `UpdateQueuedAck` calls + `client.UpdatePost`. The token is the post ID. + +- [x] 7.5 Add unit tests for each adapter's `SendQueuedAck` / `UpdateQueuedAck`: mock + the platform API client, call the methods, assert the correct API call was made with the + expected message text and the token returned / consumed correctly. + +## 8. Queued-ack lifecycle in `handleInbound` + +- [x] 8.1 In `handleInbound` (`dispatch.go:184`), after the active-agent check and + before the parts subscription, introduce the short-wait threshold logic (Decision 3): + arm a 2-second timer. If the initial `ag.Run` call does NOT return `ErrSessionBusy` + (i.e. the session was idle), skip all ack logic. If it does return `ErrSessionBusy`, + record that an ack is pending. + +- [x] 8.2 When the 2-second timer fires (i.e. the retry loop is still in progress), check + `cfg.Router.QueueAcknowledgementsEnabled`. If true, cast the adapter to + `QueuedAcknowledger` and call `SendQueuedAck(ctx, in.Peer, currentPosition)`. Store the + returned token. `currentPosition` is determined by reading `len(d.inbound)` at the time + of enqueueing — the count of messages already queued ahead. + +- [x] 8.3 On each retry cycle (after each 100 ms backoff), if a token exists, call + `UpdateQueuedAck` with the updated position. Position decreases by 1 for each message + the dispatcher processes ahead of this one (monotonically decreasing). Position 1 means + "you're next". Failures to update are logged at debug level; do not abort the retry. + +- [x] 8.4 When the retry loop exits (either `agent.Run` succeeds or returns a non-busy + error), if a token exists: call `UpdateQueuedAck` with a "running" state text (e.g. + "▶ Processing your message now…") or call a `DeleteQueuedAck` / pass a sentinel position + (0 or -1) that the adapter interprets as "resolve". Choose whichever is cleaner for the + adapter interface; document the convention in the interface definition. + +- [x] 8.5 Add a test `TestHandleInbound_QueuedAckLifecycle`: mock adapter implements + `QueuedAcknowledger`; mock agent returns `ErrSessionBusy` 3 times then succeeds; + assert (a) `SendQueuedAck` called once after the threshold, (b) `UpdateQueuedAck` called + 3 times with decreasing position, (c) `UpdateQueuedAck` called once more with the + "resolved" sentinel on success. Run with `go test -race`. + +## 9. Test coverage gaps (serialization and busy path) + +The following tests close gaps identified in the dossier — the serialization invariant and +`ErrSessionBusy`-from-competing-actor path are currently unpinned: + +- [x] 9.1 `TestDispatch_SerializationInvariant` (covered by task 1.7 above) — ensure + exactly one Run in flight at any time. + +- [x] 9.2 `TestBufferInbound_NoDrainWithoutQuestion`: verify that when an interactive + session has pending buffered messages and a question arrives, `BufferInbound` is drained + in FIFO order into the next Ask call. + +- [x] 9.3 Ensure `go test -race ./internal/bridge/service/ ./internal/config/` is green + after all above changes. + +## 10. Verification + +- [ ] 10.1 End-to-end: start a flow step against a session bound to a chat peer; send a + chat message while the step owns the session slot; confirm (a) the message is delivered + to the agent after the step completes (not discarded), (b) the queued-ack appears and + is resolved when the step finishes. + +- [ ] 10.2 Saturate `POST /router/inbound`: fill `inboundCh` to cap 64 and POST; assert + response is 429 with `Retry-After: 1` header and `dispatcherSaturated: true` body. + +- [x] 10.3 Confirm `go build ./...` clean and `go vet ./...` clean after all changes. diff --git a/openspec/changes/queue-user-messages-while-busy/.openspec.yaml b/openspec/changes/queue-user-messages-while-busy/.openspec.yaml new file mode 100644 index 0000000000..34f54d2e6c --- /dev/null +++ b/openspec/changes/queue-user-messages-while-busy/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-09-05 diff --git a/openspec/changes/queue-user-messages-while-busy/design.md b/openspec/changes/queue-user-messages-while-busy/design.md new file mode 100644 index 0000000000..60a9b40b8a --- /dev/null +++ b/openspec/changes/queue-user-messages-while-busy/design.md @@ -0,0 +1,188 @@ +# Design: queue-user-messages-while-busy + +## Context + +See `proposal.md — Why` for motivation. The relevant current state is: + +- `editorCmp.send()` (`internal/tui/components/chat/editor.go:140`) returns a warning + toast and does nothing else when `IsSessionBusy` is true. The textarea text survives + but the submission is discarded. +- The process-global session-run ledger (`internal/llm/agent/session_locks.go`) enforces + one `RunWith` goroutine per session; any second attempt returns `ErrSessionBusy` with + no side effects. This invariant is never relaxed. +- The agent loop's `processGeneration` re-reads the DB message log on compaction + (`agent.go:945`) and in the non-interactive outer cycle (`agent.go:1244`). A message + persisted to the DB before delivery would appear in those reloads non-deterministically. +- The bridge dispatcher (`internal/bridge/service/dispatch.go`) already solves a + structurally identical problem: `sessionDispatch.run()` processes one `handleInbound` + at a time, blocking for the full `agent.Run` lifetime; concurrent inbound messages + queue on a channel rather than being rejected. +- Consecutive same-role user turns are combined by the direct Anthropic API, historically + rejected by Bedrock, and unverified on VertexAI. Injecting a queued message mid-run + (between tool calls) would require merging it into the tool-result user message — each + provider presents a distinct correctness risk, and the blast radius on the hottest code + path is high. + +## Goals / Non-Goals + +**Goals:** + +- Zero lost submissions: every Enter keypress while busy enqueues the text. +- FIFO delivery as separate `agent.Run` calls after the current run completes. +- No goroutine leaks; no blocking of the Bubble Tea render loop. +- A clear, discard-capable TUI affordance. +- Full compliance with the `session-run-exclusivity` invariant. + +**Non-Goals:** + +- Mid-run injection (between tool calls). Deferred follow-up; the Bedrock alternation + constraint makes this non-trivial and the blast radius is high. Documented here so the + follow-up starts informed. +- Queuing Ctrl+E (open external `$EDITOR`) or slash commands. These mutate session state + in ways that do not compose safely with a queued text turn. +- Persistence of queued messages. The in-memory approach is intentional. +- Bounding the queue. Unbounded slice; a future change may impose a cap if real-world + memory pressure appears, and MUST specify overflow back-pressure at that point. + +## Decisions + +### Decision 1 — Option A (post-run delivery) over B (mid-run injection) and C (shared dispatcher) + +**Option A** keeps all queued messages in memory and delivers each one as a separate +`agent.Run` after the slot frees. No agent-loop change; no provider risk; fully +compliant with `session-run-exclusivity`. This is what Claude Code does: queued input +lands after the current response completes, never between tool calls. + +**Option B** (mid-run turn-boundary injection, delivering mid-turn before the next +`streamAndHandleEvents` call) avoids the extra round-trip but has two hard problems: +(1) Bedrock rejects consecutive user turns, so the queued text would need to be merged +into the same user message as the pending tool results — non-trivial merge logic in the +hottest code path; (2) ordering relative to tool results is ambiguous and changes model +behavior in ways that are hard to test. Scoped explicitly as a follow-up. + +**Option C** (extract the bridge `sessionDispatch` into `internal/dispatch/` and use it +from both bridge and TUI) would eliminate duplication. The bridge path is proven and +load-bearing, so touching it carries risk. The queue-and-serialize core is ~40 lines; +duplicating it in `internal/app/` is simpler and safer for this change. A future +refactor can unify them once both sides are stable. + +**Chosen: Option A.** Smallest blast radius, correct semantics, reviewable on its own. + +### Decision 2 — In-memory queue, not persisted-pending-message + +Adding a `Pending bool` or `Status` column to `message.Message` would require a schema +migration, a struct change, and updates to all consumers of the message list. More +critically, anything written to the DB is immediately visible to the compaction reload +and the non-interactive outer loop in `processGeneration`, making delivery +non-deterministic. An in-memory queue avoids both the schema churn and the hazard. +Consequence: undelivered messages are silently dropped on app exit — acceptable because +the session was never committed. + +### Decision 3 — Unbounded slice, not a capped channel + +The bridge uses `chan bridge.Inbound` (cap 16) with blocking back-pressure. A channel +with blocking push would stall the Bubble Tea `Update` goroutine, which processes every +key event; a TUI event loop MUST NOT block on an application concern. An unbounded slice +with non-blocking append is safe. A real-world user typing faster than the agent can drain +is unlikely to exhaust memory, and an explicit overflow requirement is deferred. + +### Decision 4 — Cancel preserves the queue; discard is explicit + +Esc/Ctrl+C fires the run's cancel func and does NOT touch the queue. Rationale: the most +common interruption pattern is "I noticed a mistake; let me redirect" — the queued +follow-up message IS the redirection. The counter-argument (a user hitting Esc to "stop +everything" is surprised when a new run starts) is addressed by making the discard +affordance prominent and by displaying the queue count while the session is busy, so the +user knows messages are pending before pressing Esc. + +### Decision 5 — Drain worker per session, on first enqueue + +Starting the worker lazily (on first enqueue) avoids creating goroutines for sessions +that never queue anything. The worker is cancelled via a `context.WithCancel` tied to +the session's lifetime in `app.App`. When the queue empties and no new enqueue arrives, +the worker exits; a subsequent enqueue starts a new worker. The goroutine map is guarded +by a mutex on `App`. + +### Decision 6 — ErrSessionBusy is retried with back-off, not surfaced + +The drain worker will lose an `acquireSessionSlot` race against cron commits. Surfacing +`ErrSessionBusy` as an error toast would alarm the user for a condition that is +transparently retried. The worker re-queues the failed message at the head and sleeps a +short back-off (e.g. 100 ms) before retrying. The retry count is not bounded — the +surrounding context cancellation is the sole deadline. + +`IsSessionBusy` MAY be used as a non-authoritative poll to reduce pointless `Run` calls +(avoid attempting when the session is observably busy), but MUST NOT be treated as the +correctness gate for exclusivity — a race can make it return false while the slot is +still held by the deferred cleanup of a run goroutine. The authoritative mechanism is the +atomic `LoadOrStore` in `acquireSessionSlot` and the `ErrSessionBusy` it returns to any +loser. The implementation MUST NOT introduce a check-then-act pattern. + +### Decision 8 — Enqueue whenever the queue is non-empty, even when the slot is idle + +The `send()` routing condition is: **enqueue if `QueueLen(sessionID) > 0` OR +`IsSessionBusy(sessionID)`; dispatch directly only when both are false** (queue empty AND +slot observably idle). Without this, a message submitted in the window between two drain +deliveries (queue non-empty, slot temporarily free) bypasses the head of the queue and +causes a FIFO inversion — the new message starts a Run before the already-queued +message. The queue-non-empty branch is checked first, before the busy check, so the +ordering is stable even if the slot races. + +Consequence: the direct idle path is preserved exactly for the common case (queue always +empty on an idle session). Queue involvement adds no latency when the queue is empty. + +### Decision 7 — No shared primitive extracted from the bridge + +The bridge's `sessionDispatch` carries bridge-only concerns: inbound channel, parts +fan-out to chat peers, tool-update streaming to Telegram/Slack/Mattermost. Extracting the +~40-line queue-and-serialize core requires splitting the type, adjusting import paths, and +retesting the bridge path. Given the risk/reward ratio, this change duplicates the +pattern in `internal/app/` and documents the follow-up extraction as a known tech-debt +item. + +### Decision 9 — Halt the drain on non-ErrSessionBusy error + +Two options: + +- **Continue**: surface the error for M1, keep draining M2, M3, … Each failing message + produces its own error toast. Advantage: one bad message (e.g. context-length rejection + unique to M1) does not block well-formed messages behind it. +- **Halt**: surface the error for M1, stop the drain worker, preserve M2 … MN in the + queue. One toast; remaining queue visible and discardable. + +**Chosen: Halt.** A systemic failure (auth error, model endpoint down, API quota hit) +would fire N identical error toasts in rapid succession with no way to stop the cascade — +far more alarming and confusing than one attributed toast followed by a visible "N still +queued" indicator. The cost is that a single malformed message (e.g. one that exceeds the +context window) blocks the messages behind it until the user discards it. This is +acceptable because: (a) such messages are rare in practice; (b) the discard affordance +is always visible so the user is never stuck; (c) the drain restarts naturally on the +next enqueue, so a transient error (network blip) recovers without user action beyond +retrying. + +## Risks / Trade-offs + +**[Risk] Undelivered messages dropped on quit** → Acceptable: sessions are best-effort; +no user-visible data loss claim is made. The TUI affordance ("N queued") warns the user +before they quit. + +**[Risk] Drain worker continues after session switch** → Intentional by Decision 5. +The worker is bound to the session, not the TUI's active session. This is correct +behavior; a background session's drain should not stop because the user opened a different +tab. + +**[Risk] Drain worker retries ErrSessionBusy indefinitely** → Bounded by the context +lifetime. If the app is shutting down, the context is cancelled and the worker exits. +Long-lived retries are not expected in practice (cron locks are short-lived). + +**[Risk] Bedrock alternation if follow-up mid-run injection is attempted** → Documented +explicitly here so the follow-up implementation is aware. Any mid-run injection MUST merge +the queued text into the tool-result user message block, NOT append a new user message. + +**[Trade-off] Queue is unbounded** → Simplicity and render-loop safety over memory +efficiency. A 10,000-message queue at ~1 KB/message is ~10 MB — unlikely in practice. + +## Open Questions + +None that would change the spec, approach, or task breakdown. The mid-run injection +design is a deliberate follow-up, not an open question for this change. diff --git a/openspec/changes/queue-user-messages-while-busy/proposal.md b/openspec/changes/queue-user-messages-while-busy/proposal.md new file mode 100644 index 0000000000..0e7c7e23b4 --- /dev/null +++ b/openspec/changes/queue-user-messages-while-busy/proposal.md @@ -0,0 +1,77 @@ +## Why + +While the agent is running, pressing Enter in the TUI discards the submission with a +transient warning toast — the typed text is preserved in the textarea, but the user must +notice when the run finishes and press Enter again manually. Claude Code queues the input +instead and delivers it after the current response completes. Users expect the same. + +## What Changes + +- **Per-session in-memory queue on `app.App`.** When `editorCmp.send()` fires while the + session is busy, the typed text and attachments are enqueued rather than rejected. The + textarea is reset (text moves into the queue), and no warning toast is shown. +- **Queue drain worker.** A goroutine per session watches for the session slot to free, + then drains the queue one message at a time — each as a separate `agent.Run` call so + user intent is preserved and ordering is FIFO. `ErrSessionBusy` from `Run` is treated + as a retryable condition, not a surfaced error. +- **TUI affordance.** The chat list (or an inline banner) shows "N message(s) queued" + while the queue is non-empty, with a dedicated key binding to discard the entire queue. +- **Explicit non-modification of `session-run-exclusivity`.** The queue never attempts a + concurrent `Run`; it only starts the next run after the slot is genuinely free. The + one-Run-per-session invariant is preserved by construction: the drain worker simply + waits for the slot to open before calling `Run`, and receives `ErrSessionBusy` as a + retryable signal rather than an error. No requirement in the `session-run-exclusivity` + spec changes. +- **OpenEditor and custom commands remain gated.** The Ctrl+E (open external `$EDITOR`) + path and `dialog.CommandRunCustomMsg` keep their busy-reject guard. External editor + sessions and slash commands mutate session state in ways that do not compose safely + with queued chat messages; queuing them is a separate, later decision. + +### Non-goals + +- Mid-run injection between tool calls is explicitly out of scope. Queued messages are + delivered only after the current run completes — not between tool calls — because the + direct Anthropic API silently combines consecutive same-role turns while Bedrock has + historically rejected them outright; VertexAI behavior is unverified. Each outcome + is a distinct correctness risk that requires dedicated investigation before mid-run + injection can be attempted safely. This non-goal must be stated in release notes so + no user or reviewer expects intra-run delivery. +- Flow-owned sessions (`NonInteractive = true`) are out of scope. The queue MUST NOT + enqueue against sessions owned by the flow engine. + +## Capabilities + +### New Capabilities + +- `chat-message-queue`: the per-session in-memory queue, its drain lifecycle (startup, + drain ordering, cancel semantics, overflow policy, session-switch and shutdown + behavior), the TUI affordance and discard key, the scope exclusions (OpenEditor, + commands, flow sessions), and the testability contract for the drain worker. + +### Modified Capabilities + + + +## Impact + +**`github.com/opencode-ai/opencode`** + +- `internal/tui/components/chat/editor.go:140-159` (`send`): replace the reject-with-toast + with an enqueue call to `app.App`; reset the textarea on successful enqueue (text moves + into the queue, so the textarea is cleared unlike today). +- `internal/tui/page/chat.go:160-167` (`Update` / `chat.SendMsg` handler): route through + the queue; `ErrSessionBusy` from the agent MUST NOT surface as a red error toast. +- `internal/tui/page/chat.go:442-457` (`sendMessage`): called by the drain worker and by + the immediate path when the session is idle; `ErrSessionBusy` here becomes retryable + via the drain, not a fatal error. +- `internal/app/app.go`: per-session queue (`[]QueuedMessage`) and drain worker goroutine, + keyed by session id; goroutine-safe map; lifecycle managed so workers do not leak across + session switches or on shutdown. +- `internal/tui/components/chat/list.go` (or a new small inline component): render "N + queued" affordance and expose a key binding to discard the queue; rendered distinctly + from persisted chat messages because queued messages are never persisted until dequeued. +- `internal/tui/page/chat.go:265-267` (Esc / cancel path): Esc cancels the in-flight run + but queued messages survive by default; the discard key is the explicit mechanism to + clear the queue. diff --git a/openspec/changes/queue-user-messages-while-busy/specs/chat-message-queue/spec.md b/openspec/changes/queue-user-messages-while-busy/specs/chat-message-queue/spec.md new file mode 100644 index 0000000000..dc4ea4dfa3 --- /dev/null +++ b/openspec/changes/queue-user-messages-while-busy/specs/chat-message-queue/spec.md @@ -0,0 +1,310 @@ +## Purpose + +Holds user chat messages submitted while the agent is running and delivers them in order +once the current run completes, giving the same queued-input experience as Claude Code +without violating the one-Run-per-session exclusivity invariant. + +## ADDED Requirements + +### Requirement: Submitted messages are enqueued rather than rejected while the session is busy + +When the user submits a message (Enter or Ctrl+S) while the session slot is held, the +system SHALL enqueue the message and attachments in an in-memory, per-session FIFO queue +and reset the textarea. The system MUST NOT show a warning toast for the discarded +submission and MUST NOT persist the queued message to the database. The textarea reset is +a behavior change from today: previously, nothing happened and the text remained in the +textarea; after this change the text moves into the queue and the input field is cleared. + +#### Scenario: Second message submitted while agent is running +- **GIVEN** the agent is running on session S +- **WHEN** the user types a message and presses Enter +- **THEN** the message is appended to S's in-memory queue, the textarea is reset, and no + toast is shown + +#### Scenario: Queue preserves FIFO order +- **GIVEN** messages M1 and M2 are enqueued on session S in that order +- **WHEN** the drain worker delivers them +- **THEN** M1 is delivered to `agent.Run` before M2 + +#### Scenario: Queued messages are not visible in the chat message list +- **GIVEN** message M is in the queue for session S +- **WHEN** the chat message list renders +- **THEN** M does not appear as a chat message (it is shown only via the queue affordance) + +### Requirement: The queue MUST NOT persist messages before delivery + +Queued messages SHALL remain in memory only until dequeued for delivery. The system MUST +NOT call `message.Create` or any DB-write path for a message while it is in the queue. +This prevents agent-loop compaction and the non-interactive reload in `processGeneration` +from sweeping a queued message into the in-flight run's history, which would make +delivery non-deterministic. + +#### Scenario: Compaction during a run with a queued message +- **GIVEN** session S has one queued message M and one in-flight run +- **WHEN** the in-flight run triggers compaction (re-listing messages from the DB) +- **THEN** M does not appear in the compacted message history; it is only delivered on + the next drain cycle after the slot frees + +#### Scenario: Queued message persisted on delivery +- **GIVEN** message M is at the head of S's queue +- **WHEN** the drain worker dequeues M and calls `agent.Run` +- **THEN** M is persisted by the normal `Run` path (as it would be for any user turn) + +### Requirement: A drain worker delivers queued messages after the slot frees + +The system SHALL maintain a per-session drain worker goroutine that calls `agent.Run` for +each queued message in order. The worker MUST treat `ErrSessionBusy` as a retryable +signal and MUST NOT surface it as a user-visible error. Each queued message MUST be +delivered as a separate `agent.Run` call (no coalescing), preserving the user's intent as +N sequential conversational turns. + +The sole correctness mechanism preventing concurrent runs is the atomic slot acquisition +inside `RunWith` (`acquireSessionSlot` `LoadOrStore`) and the `ErrSessionBusy` it returns +to any loser. `IsSessionBusy` MAY be used as a non-authoritative poll to avoid a +pointless `Run` call when the session is observably busy, but MUST NOT be relied on for +exclusivity — a race can make it return false while the slot is still held. The design +MUST NOT introduce a check-then-act pattern around slot acquisition. + +#### Scenario: Drain after a run completes +- **GIVEN** session S has queued messages [M1, M2] and the in-flight run just released + the slot +- **WHEN** the drain worker observes the slot is free +- **THEN** it calls `agent.Run(M1)`, waits for that run to complete, then calls + `agent.Run(M2)`, with each run acquiring the slot through the normal exclusivity path + +#### Scenario: ErrSessionBusy on drain is retried +- **GIVEN** the drain worker dequeues M1 and calls `agent.Run` +- **WHEN** `agent.Run` returns `ErrSessionBusy` (e.g. a cron lock beats the worker) +- **THEN** the worker re-enqueues M1 at the head of the queue and retries after a short + back-off; no error is shown to the user + +#### Scenario: Slot exclusivity is enforced by atomic acquisition, not a pre-check +- **GIVEN** session S's slot is held by an in-flight run +- **WHEN** the drain worker calls `agent.Run` (with or without a prior `IsSessionBusy` + observation, regardless of any race between the observation and the call) +- **THEN** `RunWith` returns `ErrSessionBusy` via its atomic `LoadOrStore` acquire; the + drain worker re-queues the message at the head and retries; at no point does a second + `RunWith` goroutine run on the same session concurrently + +### Requirement: Non-ErrSessionBusy errors from the drain path are surfaced and halt the drain + +When `agent.Run` returns any error other than `ErrSessionBusy`, the drain worker MUST +surface the error through the normal TUI error-reporting path. The error notification +MUST carry attribution indicating the failure originated from a queued message, so the +user understands this was not an interactive submission they just made. The drain worker +MUST halt after surfacing the first such error — it MUST NOT attempt to deliver the +remaining queued messages. The remaining queue MUST be preserved and visible in the queue +affordance, so the user can see how many messages were not delivered and choose to discard +them or allow a subsequent drain (triggered by enqueuing a new message) to retry. + +#### Scenario: Provider error on a drained message +- **GIVEN** the drain worker dequeues M1 and calls `agent.Run` +- **WHEN** `agent.Run` returns a non-`ErrSessionBusy` error (e.g. a provider API or + context-length error) +- **THEN** an attributed error notification is surfaced to the user (e.g. "Queued message + could not be delivered: "); the drain worker halts; messages M2 … MN remain in + the queue and are shown in the queue affordance + +#### Scenario: Remaining queue visible after drain halt +- **GIVEN** the drain worker halted after an error and N messages remain in the queue +- **WHEN** the chat view renders +- **THEN** the queue affordance shows N messages; the user may discard them or trigger a + fresh drain by enqueuing a new message + +### Requirement: The drain worker lifecycle is bounded and does not leak + +The system SHALL start the drain worker at most once per session on first enqueue and +MUST ensure the worker terminates when the queue is empty and no further enqueue can +arrive for that session (e.g. session switch to a new session, app shutdown). The worker +MUST NOT keep a goroutine alive after the app exits. + +#### Scenario: Worker terminates after queue is drained +- **GIVEN** session S's queue becomes empty and no new messages are enqueued +- **WHEN** the drain worker finishes delivering the last message +- **THEN** the worker goroutine exits with no leak detectable under `go test -race` + +#### Scenario: Worker is shut down on app exit +- **GIVEN** session S has a non-empty queue when the user quits +- **WHEN** the app shutdown path is reached +- **THEN** the worker goroutine is cancelled via its context; undelivered queued messages + are dropped silently (no persistence, no data-loss claim — the session was never + committed) + +### Requirement: The queue has a defined overflow policy + +The drain worker goroutine MUST NOT block the Bubble Tea render loop. The queue MUST use +an unbounded in-memory slice so a fast typist is never rejected after the first enqueue. +If memory growth is a concern in a future change, that bound and its back-pressure policy +MUST be specified explicitly in a follow-up requirement; this change intentionally leaves +the queue unbounded to avoid blocking the TUI event loop. + +#### Scenario: Many messages enqueued while agent runs +- **GIVEN** the user submits ten messages while the session is busy +- **WHEN** each Enter keypress is processed +- **THEN** all ten are enqueued with no rejection, no blocking, and no toast + +### Requirement: An explicit affordance shows queued messages and allows discard + +The TUI SHALL display a visible indicator of the form "N message(s) queued" (where N ≥ 1) +while the queue for the active session is non-empty. The indicator MUST be rendered +distinctly from chat messages (not as a persisted chat bubble). The system SHALL expose a +key binding that discards all queued messages for the active session in a single +interaction. The discard key binding MUST be shown in the status or help bar while the +queue is non-empty. + +The indicator MUST NOT add a row to the chat view: it occupies the help bar's row and the +help bar returns as soon as the queue is empty. The chat view's total height is fixed +(viewport + working row + one status row), and any extra row is clipped by the container's +MaxHeight — silently hiding whichever line is rendered last. + +#### Scenario: Queue indicator appears when session is busy and a message is queued +- **GIVEN** session S is busy and has one queued message +- **WHEN** the chat view renders +- **THEN** an indicator showing "1 message queued" (or equivalent) is visible and no + queued message appears as a chat bubble + +#### Scenario: Queue indicator disappears after drain +- **GIVEN** session S's queue is emptied by the drain worker +- **WHEN** the chat view re-renders +- **THEN** the queue indicator is no longer shown + +#### Scenario: Help bar returns when the queue empties +- **GIVEN** session S had queued messages and the queue is now empty +- **WHEN** the chat view re-renders +- **THEN** the key help bar ("press enter to send, …") is visible again, and the rendered + chat view is no taller than its assigned height + +#### Scenario: Discard key clears the queue +- **GIVEN** session S has N queued messages +- **WHEN** the user presses the discard key binding +- **THEN** all N messages are removed from the queue, the indicator disappears, and no + messages are delivered to the agent + +### Requirement: The queued messages can be inspected before delivery + +The TUI SHALL expose a key binding that toggles a viewer listing the queued messages for +the active session in delivery order. The viewer MUST read the queue at render time so it +reflects deliveries that happen while it is open, MUST render each message as a bounded +preview (single line, control sequences stripped) so a large paste cannot break the +layout, and MUST offer discard-all from inside the viewer since it captures key presses. +The binding MUST NOT be an alias of another key in terminals without the kitty keyboard +protocol (ctrl+m is Enter, ctrl+i is Tab). + +#### Scenario: Viewer lists queued messages +- **GIVEN** session S has three queued messages +- **WHEN** the user presses the view binding +- **THEN** a viewer opens listing all three previews in delivery order + +#### Scenario: Viewer reflects an ongoing drain +- **GIVEN** the viewer is open with two queued messages +- **WHEN** the drain worker delivers the first one +- **THEN** the next render of the viewer lists only the remaining message + +#### Scenario: Viewer closes on the same binding +- **GIVEN** the viewer is open +- **WHEN** the user presses the view binding again (or Esc) +- **THEN** the viewer closes and no message is sent to the agent + +### Requirement: Cancel (Esc / Ctrl+C) targets the in-flight run; queued messages survive + +Pressing Esc or Ctrl+C SHALL cancel the currently in-flight run for the active session +(per the existing cancel path) and SHALL leave queued messages intact. The drain worker +SHALL begin draining after the in-flight run's slot is released, delivering the surviving +queued messages in order. This is the default behavior because users commonly interrupt a +run precisely to let a freshly queued instruction take over. + +#### Scenario: Esc cancels in-flight run; queue survives +- **GIVEN** session S is busy and has one queued message M +- **WHEN** the user presses Esc +- **THEN** the in-flight run is cancelled, the slot is released by the run goroutine's + deferred cleanup, and M is delivered on the next drain cycle + +#### Scenario: Discard key clears the queue after cancel +- **GIVEN** the user pressed Esc and the queue still has messages +- **WHEN** the user presses the discard key +- **THEN** the remaining queued messages are dropped and no new run starts for them + +### Requirement: Flow-owned sessions and non-interactive runs are excluded + +The system MUST NOT enqueue messages against sessions owned by the flow engine +(`NonInteractive = true`). The queue affordance, the enqueue path, and the drain worker +SHALL only activate for interactive (TUI-driven) sessions. This boundary is the existing +`IsInteractiveSession` gate used by the bridge dispatcher. + +#### Scenario: Flow-session submission attempt is not queued +- **GIVEN** a session S is owned by the flow engine +- **WHEN** a user-initiated submit path fires for S (if reachable) +- **THEN** no message is enqueued in S's queue and the existing behavior is unchanged + +### Requirement: External-editor and slash-command submits remain gated + +The Ctrl+E (open external `$EDITOR`) path and `dialog.CommandRunCustomMsg` (slash-command +execution) SHALL retain their existing busy-reject guard and MUST NOT enqueue. These +paths mutate session state in ways that do not compose safely with a queued message +sequence; deferring them is a separate, explicit future decision. + +#### Scenario: Ctrl+E while busy +- **GIVEN** session S is busy +- **WHEN** the user presses Ctrl+E +- **THEN** the existing warning toast is shown and no editor is opened; no enqueue occurs + +#### Scenario: Slash command while busy +- **GIVEN** session S is busy +- **WHEN** a `CommandRunCustomMsg` is dispatched +- **THEN** the existing warning toast is shown and no command runs; no enqueue occurs + +### Requirement: A new submission routes to the queue whenever the queue is non-empty + +When a session's queue is non-empty, a new message submission MUST be appended to the +queue regardless of whether the session slot is currently held. Direct dispatch via +`agent.Run` is permitted only when the queue is empty AND the session slot is free. This +preserves FIFO ordering across the boundary between drain deliveries: a submission +arriving while the queue has entries but the slot is momentarily idle (e.g. between two +drain calls) MUST NOT bypass the already-queued messages. + +#### Scenario: Submit while queue non-empty and session momentarily idle +- **GIVEN** session S's queue contains message M1 and the slot is momentarily free + (e.g. between two drain deliveries) +- **WHEN** the user submits a new message M2 +- **THEN** M2 is appended to the queue behind M1; the drain worker delivers M1 first, + then M2; no FIFO inversion occurs + +#### Scenario: Submit while queue empty and session idle +- **GIVEN** session S's queue is empty and the session slot is free +- **WHEN** the user submits a message M +- **THEN** M is dispatched directly via `agent.Run` (today's behavior, unchanged); no + queue involvement, no added latency + +### Requirement: The idle-path submit behavior is unchanged when the queue is empty + +When the session's queue is empty and the session slot is free, pressing Enter MUST +dispatch the message via the existing path — persisting and calling `agent.Run` — with no +queue involvement and no added latency. Any error returned by `agent.Run` on this direct +idle path MUST be surfaced to the user as it is today. Only `ErrSessionBusy` returned +from the drain worker's retry loop is swallowed as a retryable signal; real errors on the +direct idle path MUST NOT be silently discarded. + +#### Scenario: Idle-path submit reaches the agent with no queue involvement +- **GIVEN** session S's queue is empty and the slot is free +- **WHEN** the user submits a message M +- **THEN** `agent.Run` is called directly; no `EnqueueMessage` call is made; the message + reaches the agent with the same latency as before this change + +#### Scenario: Real error on idle path surfaces to the user +- **GIVEN** session S's queue is empty and the slot is free +- **WHEN** the user submits a message and `agent.Run` returns a non-`ErrSessionBusy` + error +- **THEN** the error is surfaced via `util.ReportError` as it is today; it is NOT silently + swallowed + +### Requirement: Queue is per-session and does not follow session switches + +Each session MUST have its own independent queue. Switching to a different session MUST +NOT carry the original session's queue to the new session. If the original session's +drain worker is still running, it MUST continue draining against the original session +regardless of which session is currently active in the TUI. + +#### Scenario: Switch sessions while queue is non-empty +- **GIVEN** session S1 has two queued messages and the user switches to session S2 +- **WHEN** the TUI renders S2 +- **THEN** S2's queue is empty; S1's drain worker continues draining S1 in the background diff --git a/openspec/changes/queue-user-messages-while-busy/tasks.md b/openspec/changes/queue-user-messages-while-busy/tasks.md new file mode 100644 index 0000000000..c8813fa4d9 --- /dev/null +++ b/openspec/changes/queue-user-messages-while-busy/tasks.md @@ -0,0 +1,137 @@ +## 1. Per-session queue type in `internal/app/` + +- [x] 1.1 Define `QueuedMessage` struct in `internal/app/app.go` (or a new file + `internal/app/queue.go`) with fields `Text string`, `Attachments []message.Attachment`. +- [x] 1.2 Add `queues map[string][]QueuedMessage` and `queueMu sync.Mutex` to `App`; + add `queueCancels map[string]context.CancelFunc` to track per-session drain workers. +- [x] 1.3 Implement `App.EnqueueMessage(sessionID string, msg QueuedMessage)` (goroutine-safe): + append to `queues[sessionID]`, start drain worker if not already running. +- [x] 1.4 Implement `App.DequeueMessage(sessionID string) (QueuedMessage, bool)` (goroutine-safe): + pop the head of the queue, return false when empty. +- [x] 1.5 Implement `App.QueueLen(sessionID string) int` (goroutine-safe): returns + current queue length for the affordance. +- [x] 1.6 Implement `App.DiscardQueue(sessionID string)` (goroutine-safe): clear the + queue for the given session (used by the discard key binding). + +## 2. Drain worker in `internal/app/` + +- [x] 2.1 Implement `App.startDrainWorker(sessionID string)` (called under `queueMu`): + creates a `context.WithCancel` child of the app context, stores the cancel in + `queueCancels`, and launches a goroutine. +- [x] 2.2 Drain worker loop: call `agent.Run(ctx, sessionID, msg.Text, 0, msg.Attachments...)` + directly. `IsSessionBusy` MAY be used as a non-authoritative poll to skip a pointless + `Run` call when the session is observably busy (e.g. sleep 50 ms when `IsSessionBusy` + returns true), but MUST NOT be treated as the correctness gate — the authoritative + exclusivity mechanism is `ErrSessionBusy` returned by `RunWith`'s atomic acquire. + DO NOT introduce a check-then-act pattern where a false `IsSessionBusy` result is + assumed to guarantee a successful acquire. +- [x] 2.3 On `ErrSessionBusy` from `Run`: re-prepend the message to the head of the + queue (`queueMu` guarded) and apply a 100 ms back-off before the next iteration. +- [x] 2.4 On any non-`ErrSessionBusy` error from `Run`: surface the error through the + TUI error-reporting path with attribution (e.g. prefix the message with "Queued message + failed: " or equivalent so the user knows this was not an interactive submission); halt + the drain worker (exit the goroutine); preserve the remaining queue — do NOT discard it. + The worker's `queueCancels` entry MUST be removed on halt so a subsequent enqueue + starts a fresh worker. +- [x] 2.5 Worker exits when `DequeueMessage` returns false (empty queue); remove the + cancel from `queueCancels` so a subsequent enqueue starts a fresh worker. +- [x] 2.6 Worker exits on context cancellation (app shutdown or explicit stop). +- [x] 2.7 In app shutdown (`App.Close` or equivalent): cancel all live drain workers via + `queueCancels`; wait for goroutines to exit (use a `sync.WaitGroup` on workers). + +## 3. Wire `editorCmp.send()` to enqueue + +- [x] 3.1 In `internal/tui/components/chat/editor.go:140` (`send()`): replace the + `if m.app.ActiveAgent().IsSessionBusy(m.session.ID) { return util.ReportWarn(...) }` + guard with routing on `(QueueLen > 0 || IsSessionBusy)`: if either condition is true, + call `m.app.EnqueueMessage(m.session.ID, QueuedMessage{...})`, reset the textarea, + clear attachments, return nil. If both are false (queue empty AND session idle), fall + through to the existing direct dispatch path unchanged. +- [x] 3.2 Ensure `m.textarea.Reset()` and `m.attachments = nil` execute before the + enqueue returns to the caller — text has moved into the queue, not lost. +- [x] 3.3 Verify that an empty-value submit while busy or queue non-empty is a no-op + (the empty-string guard at `editor.go:150` must run before or within the routing check + so an empty submit is always discarded regardless of queue state). +- [x] 3.4 Verify that when the queue is empty AND the session is idle, `send()` takes + the existing direct dispatch path with no `EnqueueMessage` call — idle-path latency + and error surfacing are unchanged. + +## 4. Suppress `ErrSessionBusy` toast in `sendMessage` + +- [x] 4.1 In `internal/tui/page/chat.go:442` (`sendMessage`): when `activeAgent.Run` + returns `ErrSessionBusy`, return nil (no error toast) instead of + `util.ReportError(err)`. Add an `errors.Is(err, agent.ErrSessionBusy)` guard. +- [x] 4.2 Verify the change does not suppress genuinely unexpected errors — only + `ErrSessionBusy` is silenced; all other errors still route to `util.ReportError`. + +## 5. TUI queue affordance + +- [x] 5.1 Add a `QueueCountMsg` (or equivalent) Bubble Tea message that the drain worker + and `EnqueueMessage` emit to update the TUI's queue count for a session. + (Implemented as `app.DrainEvent`, delivered via `SetDrainNotifier` → `program.Send`.) +- [x] 5.2 In `internal/tui/components/chat/list.go` (or a new + `internal/tui/components/chat/queue_banner.go`): render an inline banner "N message(s) + queued — press to discard" when `QueueLen(sessionID) > 0`. The banner MUST be + styled distinctly from chat messages (e.g., a muted/info color, not a bubble). +- [x] 5.3 Add a `DiscardQueue` key binding for the discard action. `ctrl+d` MUST NOT be + used — it is bound to `KeyMap.InsertNewline` in the bubbles v2 textarea default KeyMap + and would collide since unhandled keys fall through to `m.textarea.Update`. Before + adopting any candidate key, verify it is absent from all three sources: `editorMaps` + (Send, OpenEditor), `DeleteKeyMaps` (AttachmentDeleteMode, Escape, DeleteAllAttachments) + — both defined in `editor.go` — and the textarea's default `KeyMap` from bubbles v2 + (`charm.land/bubbles/v2/textarea`). Wire the chosen binding in `internal/tui/page/chat.go` + to call `p.app.DiscardQueue(p.session.ID)` and emit a `QueueCountMsg{Count: 0}`. + (Chosen key: `ctrl+x`. Verified absent from editorMaps, DeleteKeyMaps, messageKeys, + and the bubbles v2 textarea default KeyMap.) +- [x] 5.4 Show the discard key in the editor's help/status bar while the queue is + non-empty (update `internal/tui/components/chat/editor.go` `View()` or the chat page's + help bar). (Shown in `queueBanner()` in list.go: "N messages queued — press ctrl+x to discard".) +- [x] 5.5 Hide the banner when `QueueLen == 0` (drain completed or discard pressed). + +## 6. Preserve OpenEditor and command guards + +- [x] 6.1 Confirm that `internal/tui/components/chat/editor.go:384-387` (Ctrl+E / + OpenEditor path) retains its `IsSessionBusy` warn-and-return with no change. +- [x] 6.2 Confirm that `internal/tui/page/chat.go:169-171` (`dialog.CommandRunCustomMsg` + handler) retains its `IsBusy()` warn-and-return with no change. +- [x] 6.3 Add a comment at each guard site noting that queueing these paths is a future + decision and must not be folded into this change. + +## 7. Tests + +- [x] 7.1 `internal/app/queue_test.go` (new): unit test `EnqueueMessage` + `DequeueMessage` + FIFO order; `DiscardQueue` empties the queue; `QueueLen` tracks correctly; concurrent + enqueue/dequeue under `go test -race` — no data race. +- [x] 7.2 `internal/app/drain_test.go` (new): drain worker with a fake `agent.Service` + mock (use `go generate ./...` mocks or inline stub): + - A second `EnqueueMessage` while the fake is "busy" does not call `Run` until the + fake marks itself idle. + - FIFO: two queued messages result in two sequential `Run` calls in enqueue order. + - `ErrSessionBusy` from `Run` causes retry (second `Run` call on the same message) + without surfacing an error; the message is not lost. + - A non-`ErrSessionBusy` error from `Run` causes the worker to surface an attributed + error notification, halt (worker goroutine exits, `queueCancels` entry removed), and + leave the remaining queue intact; a fresh `EnqueueMessage` starts a new worker. + - Worker goroutine terminates after the queue drains (use + `goleak.VerifyNone` or a `WaitGroup` assertion). + - Context cancellation (app shutdown) terminates the worker even when the queue is + non-empty. +- [x] 7.3 `internal/tui/components/chat/editor_busy_test.go` (new): construct a minimal + `editorCmp` backed by a stub `App`; assert that `send()` while busy enqueues, resets + the textarea, and returns nil (no warning `tea.Cmd`). +- [x] 7.6 `internal/app/drain_test.go` additions: test that a submission arriving while + the queue is non-empty but the slot is momentarily idle is enqueued (not dispatched + directly) and delivered after the already-queued message — FIFO is preserved across + the idle window between drain deliveries. +- [x] 7.7 `internal/tui/components/chat/editor_busy_test.go` additions: + - `send()` while queue is empty AND session idle calls `agent.Run` directly (no + `EnqueueMessage`), matching the pre-change behavior. + - `send()` while queue is non-empty but session is idle enqueues (does NOT dispatch + directly), preserving FIFO. + - A non-`ErrSessionBusy` error from `agent.Run` on the idle path surfaces to the TUI + (returns a non-nil `tea.Cmd` / `util.ReportError`). +- [x] 7.8 `internal/llm/agent/session_locks_test.go`: run unchanged; confirm green with + `go test -race ./internal/llm/agent/`. +- [x] 7.9 Run `go test -race ./internal/app/ ./internal/tui/... ./internal/llm/agent/` + and confirm all pass. +- [x] 7.10 Run `make test` per `CLAUDE.md` final-check requirement.