From 2a709ff0dbba7b0326b9b7a36da083abfa5207a8 Mon Sep 17 00:00:00 2001 From: Artem Obukhov Date: Thu, 6 Aug 2026 21:48:38 +0400 Subject: [PATCH 1/2] feat(bridge): nudge on idle question + acknowledge typed answers (GENAI-151) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Interactive question round-trips could hang silently to the job deadline when a reviewer's answer was lost in transit — e.g. a bridge reply delivered to a competing Socket Mode consumer of the same Slack app is dropped, so question.Ask never wakes. Two resilience improvements, both working in daemon and orchestrator-mediated deployments (outbound is always pod-side): - Idle "still waiting" nudge: QuestionRouter re-posts a short status to the bound peers of a session with an outstanding question after an idle gap (default 5m, capped at 3, configurable via router.questionNudgeIntervalSeconds / questionNudgeMax; negative interval disables). Re-surfaces a lost answer instead of hanging. - Typed-answer acknowledgment: a free-text / @mention / modal answer gets a brief "got it" confirmation; a button click is skipped (it already self-renders a "checked Answered" widget). Driven by a new Inbound.Source field; unknown/absent source is treated as already-acked so a button from an older orchestrator is never double-acknowledged. Adds 10 tests. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/bridge/bridge.go | 47 ++++ internal/bridge/config.go | 16 ++ internal/bridge/inbound_source_test.go | 30 +++ internal/bridge/service/question.go | 163 ++++++++++++++ .../bridge/service/question_nudge_ack_test.go | 206 ++++++++++++++++++ internal/bridge/slack/adapter.go | 10 + 6 files changed, 472 insertions(+) create mode 100644 internal/bridge/inbound_source_test.go create mode 100644 internal/bridge/service/question_nudge_ack_test.go diff --git a/internal/bridge/bridge.go b/internal/bridge/bridge.go index cbfd2599a6..a8e6107d77 100644 --- a/internal/bridge/bridge.go +++ b/internal/bridge/bridge.go @@ -105,6 +105,53 @@ type Inbound struct { // CommandArgs is the remainder of the message after the command name, // if Command is set. CommandArgs string `json:"commandArgs,omitempty"` + // Source classifies how the adapter produced this inbound (see the + // InboundSource* constants). The question router reads it to decide + // whether the transport already gave the reviewer visual feedback — + // a button click self-renders a "✓ Answered" widget, so no extra + // acknowledgment is sent — versus a typed/custom answer, which gets + // none otherwise and so is acknowledged explicitly. Empty when the + // producing adapter (or an older orchestrator, over /router/inbound) + // didn't set it; treated as "unknown", which SUPPRESSES the extra ack + // so an unstamped button click is never double-acknowledged. + Source string `json:"source,omitempty"` +} + +// InboundSource classifies how a reviewer's inbound was produced. It rides +// on Inbound.Source (JSON `source`) across the /router/inbound wire so the +// orchestrator-mediated path and the daemon (adapter-owned) path agree on +// whether an explicit answer acknowledgment is warranted. +const ( + // InboundSourceButton is a question-UI block-actions button click. The + // adapter/orchestrator already rewrote the message to "✓ Answered", so + // the question router does NOT send a separate acknowledgment. + InboundSourceButton = "button" + // InboundSourceModal is a custom-answer modal submit (view_submission). + // No widget confirmation is rendered for it, so it IS acknowledged. + InboundSourceModal = "modal" + // InboundSourceMessage is a free-text DM / channel-thread message. No + // widget feedback exists for it, so a typed answer IS acknowledged. + InboundSourceMessage = "message" + // InboundSourceAppMention is an @-mention of the bot in a channel. Like + // a plain message, a typed answer via mention IS acknowledged. + InboundSourceAppMention = "appmention" +) + +// AnswerWasAcknowledgedByTransport reports whether the transport that +// produced this inbound already gave the reviewer visible confirmation of +// their answer (today: only the interactive button, which self-renders a +// "✓ Answered" widget). When true the question router skips its own +// acknowledgment to avoid double feedback. Unknown/empty Source returns +// true (conservative: suppress the extra ack rather than risk double-acking +// an unstamped button click from an older orchestrator). +func (in Inbound) AnswerWasAcknowledgedByTransport() bool { + switch in.Source { + case InboundSourceModal, InboundSourceMessage, InboundSourceAppMention: + return false + default: + // InboundSourceButton and any unknown/empty value. + return true + } } // Outbound is the normalized representation of an outbound message the diff --git a/internal/bridge/config.go b/internal/bridge/config.go index fe285c0671..7253d67277 100644 --- a/internal/bridge/config.go +++ b/internal/bridge/config.go @@ -33,6 +33,22 @@ type Config struct { // router_send agent tool to specific peers. NOT enforced in v1 (see // chat-bridge-agent-tool spec). Schema-defined but ignored at runtime. AgentPeerAllowlist []PeerRef `json:"agentPeerAllowlist,omitempty"` + + // QuestionNudgeIntervalSeconds is the idle gap after which the question + // router re-posts a short "still waiting for your answer" status to the + // bound peers of a session with an outstanding question, and the spacing + // between subsequent nudges. It re-surfaces a question whose answer was + // lost in transit (e.g. a misrouted bridge reply) instead of letting the + // interactive step sit silent until the job's hard deadline. + // 0 → use the built-in default (QuestionNudgeDefaultInterval) + // <0 → disable nudging entirely + QuestionNudgeIntervalSeconds int `json:"questionNudgeIntervalSeconds,omitempty"` + + // QuestionNudgeMax caps how many nudges are sent for a single pending + // question (so a walked-away reviewer can't be pinged forever). + // 0 → use the built-in default (QuestionNudgeDefaultMax) + // <0 → unlimited (bounded in practice by the job deadline) + QuestionNudgeMax int `json:"questionNudgeMax,omitempty"` } // ChannelsConfig holds per-platform channel sections. diff --git a/internal/bridge/inbound_source_test.go b/internal/bridge/inbound_source_test.go new file mode 100644 index 0000000000..35afa12958 --- /dev/null +++ b/internal/bridge/inbound_source_test.go @@ -0,0 +1,30 @@ +package bridge + +import "testing" + +// TestAnswerWasAcknowledgedByTransport pins the ack-suppression truth table +// that the question router relies on (GENAI-151): only sources that leave the +// reviewer with NO visible confirmation get an explicit acknowledgment; a +// button (self-renders "✓ Answered") and any unknown/empty source are treated +// as already-acked so an unstamped inbound from an older orchestrator is never +// double-acknowledged. +func TestAnswerWasAcknowledgedByTransport(t *testing.T) { + t.Parallel() + cases := []struct { + source string + alreadyAckd bool + }{ + {InboundSourceButton, true}, // widget already shows "✓ Answered" + {"", true}, // unknown / unstamped → suppress (never double-ack a button) + {"someFutureSource", true}, // unknown → suppress + {InboundSourceMessage, false}, // typed DM/channel answer → ack + {InboundSourceAppMention, false}, // @mention answer → ack + {InboundSourceModal, false}, // custom-answer modal submit → ack + } + for _, c := range cases { + got := Inbound{Source: c.source}.AnswerWasAcknowledgedByTransport() + if got != c.alreadyAckd { + t.Errorf("source=%q AnswerWasAcknowledgedByTransport()=%v, want %v", c.source, got, c.alreadyAckd) + } + } +} diff --git a/internal/bridge/service/question.go b/internal/bridge/service/question.go index ac25905d7d..ae7f666a4c 100644 --- a/internal/bridge/service/question.go +++ b/internal/bridge/service/question.go @@ -19,6 +19,20 @@ const ( recentlyAnsweredTTL = 30 * time.Second recentlyAnsweredSweepInterval = 10 * time.Second + // questionNudgeDefaultInterval is the idle gap before the first "still + // waiting" nudge, and the spacing between subsequent nudges, when the + // operator hasn't overridden cfg.QuestionNudgeIntervalSeconds. Chosen so + // a reviewer who stepped away is reminded a few times well inside a + // typical job deadline, without pinging on every brief pause. + questionNudgeDefaultInterval = 5 * time.Minute + // questionNudgeDefaultMax caps nudges per pending question when the + // operator hasn't overridden cfg.QuestionNudgeMax. + questionNudgeDefaultMax = 3 + // questionNudgeSweepInterval is how often the nudger wakes to look for + // questions that have gone quiet past the interval. Kept well below the + // nudge interval so a due nudge fires promptly without busy-looping. + questionNudgeSweepInterval = 30 * time.Second + // interactiveInboundBufferCap bounds the per-session queue of reviewer // messages buffered while an interactive flow step has no question // pending (see BufferInbound). Drop-oldest once the cap is hit so a @@ -76,6 +90,13 @@ type QuestionRouter struct { type pendingQuestion struct { requestID string prompts []question.Prompt + // askedAt is when the question was surfaced; sinceLast tracks the + // nudger's clock. nudges counts how many "still waiting" reminders + // have been sent so the per-question cap is enforced. Guarded by + // QuestionRouter.mu. + askedAt time.Time + lastNudge time.Time + nudges int } // NewQuestionRouter constructs a router and starts the subscriber @@ -89,6 +110,7 @@ func (s *Service) newQuestionRouter() *QuestionRouter { if s.app != nil && s.app.Questions != nil { s.launchSupervised("question-router", r.run) s.launchSupervised("question-stale-cache-sweeper", r.runSweeper) + s.launchSupervised("question-nudger", r.runNudger) } return r } @@ -208,6 +230,7 @@ func (r *QuestionRouter) handleNewRequest(ctx context.Context, req question.Requ pend := &pendingQuestion{ requestID: req.ID, prompts: req.Questions, + askedAt: time.Now(), } r.mu.Lock() r.pending[req.SessionID] = pend @@ -375,9 +398,47 @@ func (r *QuestionRouter) TryHandleQuestionReply(ctx context.Context, sessionID s return false } r.rememberAnswers(sessionID, answers) + r.maybeAckAnswer(ctx, in, answers) return true } +// maybeAckAnswer sends a short confirmation for a typed/custom answer that +// got NO transport-side feedback. A button click already self-renders a +// "✓ Answered" widget (InboundSourceButton, or an unstamped inbound from an +// older orchestrator — treated conservatively as already-acked), so it is +// skipped; a free-text @mention / DM / modal answer otherwise leaves the +// reviewer with no sign the agent received it. Sent to the answering peer +// via the pod's own outbound adapter, so it works in both daemon and +// orchestrator-mediated deployments. Best-effort — a delivery failure never +// blocks the (already-committed) Reply. +func (r *QuestionRouter) maybeAckAnswer(ctx context.Context, in bridge.Inbound, answers [][]string) { + if in.AnswerWasAcknowledgedByTransport() { + return + } + if ack := formatAnswerAck(answers); ack != "" { + r.svc.replyToPeer(ctx, in.Peer, ack) + } +} + +// formatAnswerAck renders the short confirmation sent back to a reviewer +// who typed/mentioned a custom answer. Lists the recorded answer(s) so it's +// unambiguous what the agent captured; returns "" when there's nothing to +// echo (defensive — the caller then skips the send). +func formatAnswerAck(answers [][]string) string { + var picks []string + for _, row := range answers { + for _, a := range row { + if a = strings.TrimSpace(a); a != "" { + picks = append(picks, a) + } + } + } + if len(picks) == 0 { + return "" + } + return "👍 Got it — recorded your answer: " + strings.Join(picks, ", ") + ". Working on it…" +} + // BufferInbound queues a reviewer message that arrived for an // interactive flow session while no question was pending. The next // question the flow agent asks (handleNewRequest) is auto-answered from @@ -431,6 +492,108 @@ func (r *QuestionRouter) ClearSession(sessionID string) { delete(r.buffered, sessionID) } +// runNudger periodically re-surfaces questions that have gone unanswered +// past the nudge interval, re-posting a short "still waiting" status to the +// session's bound peers. This turns a silently-lost answer (e.g. a bridge +// reply misrouted to another Socket Mode consumer, GENAI-151) into a +// visible, recoverable prompt instead of an interactive step that hangs to +// the job's hard deadline. Stops when the service context cancels. +func (r *QuestionRouter) runNudger(ctx context.Context) { + ticker := time.NewTicker(questionNudgeSweepInterval) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return + case now := <-ticker.C: + r.nudgeDue(ctx, now) + } + } +} + +// nudgeConfig resolves the effective nudge settings from cfg, applying +// defaults for zero values. A negative interval disables nudging; a +// negative max means unlimited (bounded in practice by the job deadline). +func (r *QuestionRouter) nudgeConfig() (interval time.Duration, max int, enabled bool) { + interval = questionNudgeDefaultInterval + max = questionNudgeDefaultMax + if r.svc != nil && r.svc.cfg != nil { + switch s := r.svc.cfg.QuestionNudgeIntervalSeconds; { + case s < 0: + return 0, 0, false + case s > 0: + interval = time.Duration(s) * time.Second + } + if m := r.svc.cfg.QuestionNudgeMax; m != 0 { + max = m + } + } + return interval, max, true +} + +// nudgeDue sends a reminder for every pending question idle past the +// interval and under the per-question cap. The pending map is scanned under +// the lock to collect the due-list — updating each entry's counters in the +// SAME critical section so a concurrent reply/sweep can't double-send — then +// the chat I/O happens after the lock is released. +func (r *QuestionRouter) nudgeDue(ctx context.Context, now time.Time) { + interval, max, enabled := r.nudgeConfig() + if !enabled { + return + } + type dueNudge struct { + sessionID string + prompts []question.Prompt + } + var todo []dueNudge + r.mu.Lock() + for sid, p := range r.pending { + // Reference clock: the last nudge, or the ask time if none sent yet. + last := p.lastNudge + if last.IsZero() { + last = p.askedAt + } + if now.Sub(last) < interval { + continue + } + if max >= 0 && p.nudges >= max { + continue + } + p.nudges++ + p.lastNudge = now + todo = append(todo, dueNudge{sessionID: sid, prompts: p.prompts}) + } + r.mu.Unlock() + + for _, d := range todo { + text := formatNudge(d.prompts) + if text == "" { + continue + } + if _, err := r.svc.SendBySessionID(ctx, d.sessionID, bridge.Outbound{Text: text}); err != nil { + logging.Warn("bridge: question nudge send failed", + "session", d.sessionID, "err", err) + continue + } + logging.Info("bridge: question nudge sent", "session", d.sessionID) + } +} + +// formatNudge renders the short "still waiting" status. Includes the first +// prompt's question text so the reviewer knows exactly what's outstanding; +// returns "" when there's nothing to echo (caller skips the send). +func formatNudge(prompts []question.Prompt) string { + if len(prompts) == 0 { + return "" + } + q := strings.TrimSpace(prompts[0].Question) + if q == "" { + return "" + } + return "⏳ Still waiting on your answer to continue:\n> " + q + + "\nReply with a button/number above, or @mention me with your answer." +} + // renderQuestionPrompt formats one or more question.Prompt entries as // numbered-option chat text. Format mirrors the TS bridge's fallback // rendering so the user experience is unchanged. diff --git a/internal/bridge/service/question_nudge_ack_test.go b/internal/bridge/service/question_nudge_ack_test.go new file mode 100644 index 0000000000..a519cee2cd --- /dev/null +++ b/internal/bridge/service/question_nudge_ack_test.go @@ -0,0 +1,206 @@ +package service + +import ( + "context" + "strings" + "testing" + "time" + + "github.com/opencode-ai/opencode/internal/bridge" + "github.com/opencode-ai/opencode/internal/question" +) + +// --- custom-answer acknowledgment (GENAI-151) --------------------------- + +// newAckRouter wires a router + a registered slack/default adapter so +// maybeAckAnswer's replyToPeer has somewhere to deliver. +func newAckRouter(t *testing.T) (*QuestionRouter, *stubAdapter) { + t.Helper() + svc, _ := newOrchestratorForTest(t) + if err := svc.Start(context.Background()); err != nil { + t.Fatalf("Start: %v", err) + } + ad := newStubAdapter("slack", "default") + if err := svc.RegisterAdapter(context.Background(), ad); err != nil { + t.Fatalf("RegisterAdapter: %v", err) + } + return &QuestionRouter{svc: svc, pending: map[string]*pendingQuestion{}}, ad +} + +func TestMaybeAckAnswer_TypedAnswersAreAcknowledged(t *testing.T) { + t.Parallel() + for _, src := range []string{bridge.InboundSourceMessage, bridge.InboundSourceAppMention, bridge.InboundSourceModal} { + r, ad := newAckRouter(t) + in := bridge.Inbound{ + Peer: bridge.PeerRef{Channel: "slack", Identity: "default", PeerID: "C1|170.5"}, + Text: "the meta service README", + Source: src, + } + r.maybeAckAnswer(context.Background(), in, [][]string{{"the meta service README"}}) + sends := ad.Sends() + if len(sends) != 1 { + t.Fatalf("source=%s: sends=%d, want 1 (typed answer must be acknowledged)", src, len(sends)) + } + if !strings.Contains(sends[0].Text, "Got it") || !strings.Contains(sends[0].Text, "meta service README") { + t.Errorf("source=%s: ack text = %q", src, sends[0].Text) + } + if sends[0].Peer.PeerID != "C1|170.5" { + t.Errorf("source=%s: ack went to peer %q, want the answering peer C1|170.5", src, sends[0].Peer.PeerID) + } + } +} + +func TestMaybeAckAnswer_ButtonAndUnknownAreNotAcknowledged(t *testing.T) { + t.Parallel() + for _, src := range []string{bridge.InboundSourceButton, "" /* older orchestrator */} { + r, ad := newAckRouter(t) + in := bridge.Inbound{ + Peer: bridge.PeerRef{Channel: "slack", Identity: "default", PeerID: "C1|170.5"}, + Text: "Yes, CD", + Source: src, + } + r.maybeAckAnswer(context.Background(), in, [][]string{{"Yes, CD"}}) + if n := len(ad.Sends()); n != 0 { + t.Errorf("source=%q: sends=%d, want 0 (button self-renders ✓ Answered; unknown suppressed to avoid double-ack)", src, n) + } + } +} + +func TestFormatAnswerAck(t *testing.T) { + t.Parallel() + if got := formatAnswerAck(nil); got != "" { + t.Errorf("empty answers → %q, want \"\"", got) + } + if got := formatAnswerAck([][]string{{" ", ""}}); got != "" { + t.Errorf("whitespace-only answers → %q, want \"\"", got) + } + got := formatAnswerAck([][]string{{"auth"}, {"billing"}}) + if !strings.Contains(got, "auth") || !strings.Contains(got, "billing") { + t.Errorf("ack should list all recorded labels: %q", got) + } +} + +// --- idle "still waiting" nudge (GENAI-151) ----------------------------- + +// newNudgeRouter wires a router + adapter + a single bound peer for session +// S1, then plants a pending question asked `askedAgo` in the past. +func newNudgeRouter(t *testing.T, askedAgo time.Duration) (*QuestionRouter, *stubAdapter) { + t.Helper() + svc, _ := newOrchestratorForTest(t) + if err := svc.Start(context.Background()); err != nil { + t.Fatalf("Start: %v", err) + } + ad := newStubAdapter("slack", "default") + if err := svc.RegisterAdapter(context.Background(), ad); err != nil { + t.Fatalf("RegisterAdapter: %v", err) + } + if _, err := svc.Bind(context.Background(), "S1", []bridge.PeerRef{ + {Channel: "slack", Identity: "default", PeerID: "C1|170.5"}, + }); err != nil { + t.Fatalf("Bind: %v", err) + } + r := &QuestionRouter{svc: svc, pending: map[string]*pendingQuestion{}} + r.pending["S1"] = &pendingQuestion{ + requestID: "q1", + prompts: []question.Prompt{{Question: "Where should the endpoint list live?"}}, + askedAt: time.Now().Add(-askedAgo), + } + return r, ad +} + +func TestNudgeDue_FiresAfterIntervalThenRespectsSpacing(t *testing.T) { + t.Parallel() + r, ad := newNudgeRouter(t, 10*time.Minute) // idle well past the 5m default + now := time.Now() + + r.nudgeDue(context.Background(), now) + if n := len(ad.Sends()); n != 1 { + t.Fatalf("first nudge: sends=%d, want 1", n) + } + if !strings.Contains(ad.Sends()[0].Text, "Still waiting") || + !strings.Contains(ad.Sends()[0].Text, "endpoint list") { + t.Errorf("nudge text = %q", ad.Sends()[0].Text) + } + + // Immediately again: spacing not elapsed → no second nudge. + r.nudgeDue(context.Background(), now) + if n := len(ad.Sends()); n != 1 { + t.Errorf("re-nudge within interval: sends=%d, want 1 (no double-ping)", n) + } + + // After another interval: second nudge fires. + r.nudgeDue(context.Background(), now.Add(6*time.Minute)) + if n := len(ad.Sends()); n != 2 { + t.Errorf("nudge after interval elapsed: sends=%d, want 2", n) + } +} + +func TestNudgeDue_NotYetIdle(t *testing.T) { + t.Parallel() + r, ad := newNudgeRouter(t, 1*time.Minute) // only 1m idle, default is 5m + r.nudgeDue(context.Background(), time.Now()) + if n := len(ad.Sends()); n != 0 { + t.Errorf("sends=%d, want 0 (not idle long enough)", n) + } +} + +func TestNudgeDue_RespectsMaxCap(t *testing.T) { + t.Parallel() + r, ad := newNudgeRouter(t, 10*time.Minute) + r.svc.cfg.QuestionNudgeMax = 1 + now := time.Now() + r.nudgeDue(context.Background(), now) // #1 + r.nudgeDue(context.Background(), now.Add(1*time.Hour)) // capped + r.nudgeDue(context.Background(), now.Add(2*time.Hour)) // capped + if n := len(ad.Sends()); n != 1 { + t.Errorf("sends=%d, want 1 (QuestionNudgeMax=1)", n) + } +} + +func TestNudgeDue_DisabledByNegativeInterval(t *testing.T) { + t.Parallel() + r, ad := newNudgeRouter(t, 1*time.Hour) + r.svc.cfg.QuestionNudgeIntervalSeconds = -1 // disabled + r.nudgeDue(context.Background(), time.Now()) + if n := len(ad.Sends()); n != 0 { + t.Errorf("sends=%d, want 0 (nudging disabled)", n) + } +} + +func TestNudgeDue_HonoursCustomInterval(t *testing.T) { + t.Parallel() + r, ad := newNudgeRouter(t, 90*time.Second) + r.svc.cfg.QuestionNudgeIntervalSeconds = 60 // 1m, below the 90s idle + r.nudgeDue(context.Background(), time.Now()) + if n := len(ad.Sends()); n != 1 { + t.Errorf("sends=%d, want 1 (custom 60s interval, 90s idle)", n) + } +} + +func TestRunNudgerStopsOnContextCancel(t *testing.T) { + t.Parallel() + r, _ := newNudgeRouter(t, 0) + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan struct{}) + go func() { r.runNudger(ctx); close(done) }() + cancel() + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("nudger goroutine did not exit after ctx cancel") + } +} + +func TestFormatNudge(t *testing.T) { + t.Parallel() + if got := formatNudge(nil); got != "" { + t.Errorf("no prompts → %q, want \"\"", got) + } + if got := formatNudge([]question.Prompt{{Question: " "}}); got != "" { + t.Errorf("blank question → %q, want \"\"", got) + } + got := formatNudge([]question.Prompt{{Question: "Pick a project"}}) + if !strings.Contains(got, "Still waiting") || !strings.Contains(got, "Pick a project") { + t.Errorf("nudge = %q", got) + } +} diff --git a/internal/bridge/slack/adapter.go b/internal/bridge/slack/adapter.go index 550abf6ba5..56c0a4e2f2 100644 --- a/internal/bridge/slack/adapter.go +++ b/internal/bridge/slack/adapter.go @@ -546,6 +546,10 @@ func (a *Adapter) handleInteractiveCallback(ctx context.Context, callback slackg Text: value, AuthorID: callback.User.ID, ReceivedAt: time.Now().UnixMilli(), + // Button click: updateAnsweredWidget (below) rewrites the message + // to "✓ Answered", so the question router must NOT send a second + // acknowledgment. + Source: bridge.InboundSourceButton, }) // Replace the actionable widget with a confirmation. Inbound has @@ -691,6 +695,9 @@ func (a *Adapter) handleMessageEvent(ctx context.Context, ev *slackevents.Messag Attachments: atts, AuthorID: ev.User, ReceivedAt: time.Now().UnixMilli(), + // Free-text DM/message: no widget feedback, so a typed answer to a + // pending question gets an explicit acknowledgment downstream. + Source: bridge.InboundSourceMessage, }) } @@ -739,6 +746,9 @@ func (a *Adapter) handleAppMention(ctx context.Context, ev *slackevents.AppMenti Attachments: atts, AuthorID: ev.User, ReceivedAt: time.Now().UnixMilli(), + // @-mention answer: no widget feedback, so it gets an explicit + // acknowledgment downstream (see QuestionRouter.TryHandleQuestionReply). + Source: bridge.InboundSourceAppMention, }) } From 7947cd4989c05495bf66b2f25c370b642cc06b96 Mon Sep 17 00:00:00 2001 From: Artem Obukhov Date: Thu, 6 Aug 2026 22:53:28 +0400 Subject: [PATCH 2/2] fix(review):add support for remaining platforms, adjust docs --- docs/bridge.md | 2 ++ internal/bridge/mattermost/adapter.go | 5 +++++ internal/bridge/mattermost/adapter_test.go | 5 +++++ internal/bridge/service/question.go | 8 ++++---- internal/bridge/service/question_nudge_ack_test.go | 6 +++--- internal/bridge/telegram/adapter.go | 11 +++++++++++ internal/bridge/telegram/adapter_test.go | 5 +++++ internal/bridge/telegram/interactive_test.go | 5 +++++ 8 files changed, 40 insertions(+), 7 deletions(-) diff --git a/docs/bridge.md b/docs/bridge.md index 9250b3a8fa..39fa2e9fe5 100644 --- a/docs/bridge.md +++ b/docs/bridge.md @@ -78,6 +78,8 @@ Health snapshot: `curl http://127.0.0.1:3456/router/health` (per-adapter `status | Field | Values | Description | |---|---|---| | `questionMode` | `"interactive"` \| `"disabled"` | When `interactive`, the agent's `question` tool renders Slack actions blocks / Telegram inline keyboards with a numbered-text fallback. When unset or `"disabled"`, the question tool isn't initialized. | +| `questionNudgeIntervalSeconds` | `int` | Idle gap after which the bridge re-posts a "still waiting for your answer" nudge to a session's bound peers when a `question` is outstanding, and the spacing between subsequent nudges. Re-surfaces an answer lost in transit (e.g. a misrouted bridge reply) instead of letting the step hang to the job's hard deadline. `0` → built-in default (5 min); `<0` → disable nudging. | +| `questionNudgeMax` | `int` | Caps how many nudges are sent for a single pending question (so a walked-away reviewer can't be pinged forever). `0` → built-in default (3); `<0` → unlimited (bounded in practice by the job deadline). | | `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. Error lines surface regardless of this flag. | | `channels.{telegram,slack,mattermost}` | object | Per-platform configuration; see below. | diff --git a/internal/bridge/mattermost/adapter.go b/internal/bridge/mattermost/adapter.go index ff0975b296..383d81df33 100644 --- a/internal/bridge/mattermost/adapter.go +++ b/internal/bridge/mattermost/adapter.go @@ -414,6 +414,11 @@ func (a *Adapter) dispatchPosted(ctx context.Context, ev WSEvent, inbound chan<- Text: text, AuthorID: post.UserID, ReceivedAt: time.Now().UnixMilli(), + // Mattermost has no interactive question widget — every answer is a + // typed post with no transport-side feedback, so a reply to a pending + // question gets an explicit acknowledgment downstream (see + // QuestionRouter.TryHandleQuestionReply). + Source: bridge.InboundSourceMessage, } // Persist any attached files into the bridge media store. Failures diff --git a/internal/bridge/mattermost/adapter_test.go b/internal/bridge/mattermost/adapter_test.go index e629a49547..8f9f336268 100644 --- a/internal/bridge/mattermost/adapter_test.go +++ b/internal/bridge/mattermost/adapter_test.go @@ -338,6 +338,11 @@ func TestHandlesDMPostedEvents(t *testing.T) { if in.Peer.PeerID != "dm_channel_1|post1" { t.Errorf("PeerID = %q, want dm_channel_1|post1", in.Peer.PeerID) } + // Mattermost has no interactive widget → typed answer → tagged so a + // reply to a pending question gets an explicit acknowledgment. + if in.Source != bridge.InboundSourceMessage { + t.Errorf("Source = %q, want %q", in.Source, bridge.InboundSourceMessage) + } } func TestFiltersOwnMessages(t *testing.T) { diff --git a/internal/bridge/service/question.go b/internal/bridge/service/question.go index ae7f666a4c..28cce32717 100644 --- a/internal/bridge/service/question.go +++ b/internal/bridge/service/question.go @@ -90,10 +90,10 @@ type QuestionRouter struct { type pendingQuestion struct { requestID string prompts []question.Prompt - // askedAt is when the question was surfaced; sinceLast tracks the - // nudger's clock. nudges counts how many "still waiting" reminders - // have been sent so the per-question cap is enforced. Guarded by - // QuestionRouter.mu. + // askedAt is when the question was surfaced; lastNudge is when the + // nudger last re-posted a "still waiting" reminder (zero until the + // first one). nudges counts how many reminders have been sent so the + // per-question cap is enforced. Guarded by QuestionRouter.mu. askedAt time.Time lastNudge time.Time nudges int diff --git a/internal/bridge/service/question_nudge_ack_test.go b/internal/bridge/service/question_nudge_ack_test.go index a519cee2cd..fdaea64bf2 100644 --- a/internal/bridge/service/question_nudge_ack_test.go +++ b/internal/bridge/service/question_nudge_ack_test.go @@ -149,9 +149,9 @@ func TestNudgeDue_RespectsMaxCap(t *testing.T) { r, ad := newNudgeRouter(t, 10*time.Minute) r.svc.cfg.QuestionNudgeMax = 1 now := time.Now() - r.nudgeDue(context.Background(), now) // #1 - r.nudgeDue(context.Background(), now.Add(1*time.Hour)) // capped - r.nudgeDue(context.Background(), now.Add(2*time.Hour)) // capped + r.nudgeDue(context.Background(), now) // #1 + r.nudgeDue(context.Background(), now.Add(1*time.Hour)) // capped + r.nudgeDue(context.Background(), now.Add(2*time.Hour)) // capped if n := len(ad.Sends()); n != 1 { t.Errorf("sends=%d, want 1 (QuestionNudgeMax=1)", n) } diff --git a/internal/bridge/telegram/adapter.go b/internal/bridge/telegram/adapter.go index e21db01b0e..718be3c9aa 100644 --- a/internal/bridge/telegram/adapter.go +++ b/internal/bridge/telegram/adapter.go @@ -472,6 +472,10 @@ func (a *Adapter) handleCallbackQuery(ctx context.Context, cb *models.CallbackQu Text: cb.Data, AuthorID: strconv.FormatInt(cb.From.ID, 10), ReceivedAt: time.Now().UnixMilli(), + // Button click: updateAnsweredWidget (below) rewrites the message to + // "✓ Answered", so the question router must NOT send a second + // acknowledgment. + Source: bridge.InboundSourceButton, }) // Replace the inline keyboard + prefix the prompt with a confirmation @@ -579,6 +583,9 @@ func (a *Adapter) handleMultiSelectCallback(ctx context.Context, cb *models.Call Text: strings.Join(selected, ", "), AuthorID: strconv.FormatInt(cb.From.ID, 10), ReceivedAt: time.Now().UnixMilli(), + // Multi-select submit: updateAnsweredWidget (below) rewrites the + // message to "✓ Answered", so no separate acknowledgment is sent. + Source: bridge.InboundSourceButton, }) // Map state values → display labels for the confirmation prefix. labels := make([]string, 0, len(selected)) @@ -740,6 +747,10 @@ func (a *Adapter) handleMessage(ctx context.Context, msg *models.Message) { Text: cleanText, Attachments: attachments, ReceivedAt: time.Now().UnixMilli(), + // Free-text DM / stripped @mention: no widget feedback, so a typed + // answer to a pending question gets an explicit acknowledgment + // downstream (see QuestionRouter.TryHandleQuestionReply). + Source: bridge.InboundSourceMessage, } if msg.From != nil { in.AuthorID = strconv.FormatInt(msg.From.ID, 10) diff --git a/internal/bridge/telegram/adapter_test.go b/internal/bridge/telegram/adapter_test.go index 3377f270be..dd5aefc944 100644 --- a/internal/bridge/telegram/adapter_test.go +++ b/internal/bridge/telegram/adapter_test.go @@ -430,6 +430,11 @@ func TestGroupMessagesWithGroupsEnabledRequireMention(t *testing.T) { if in.Text != "please review" { t.Errorf("group inbound Text = %q, want %q", in.Text, "please review") } + // Typed message → no widget feedback → tagged so a reply to a pending + // question gets an explicit acknowledgment. + if in.Source != bridge.InboundSourceMessage { + t.Errorf("Source = %q, want %q", in.Source, bridge.InboundSourceMessage) + } } func TestPrivateModeRejectsNonAllowlistedPeer(t *testing.T) { diff --git a/internal/bridge/telegram/interactive_test.go b/internal/bridge/telegram/interactive_test.go index bc0b3ea470..cc526fd1db 100644 --- a/internal/bridge/telegram/interactive_test.go +++ b/internal/bridge/telegram/interactive_test.go @@ -145,6 +145,11 @@ func TestTelegramCallbackQueryRoutesAsInbound(t *testing.T) { if in.AuthorID != "12345" { t.Errorf("AuthorID = %q", in.AuthorID) } + // Button click self-renders "✓ Answered", so it must be tagged as such + // to suppress the downstream typed-answer acknowledgment. + if in.Source != bridge.InboundSourceButton { + t.Errorf("Source = %q, want %q", in.Source, bridge.InboundSourceButton) + } } // io import marker