feat(slack): add [slack].streaming toggle (send-once mode)#1115
feat(slack): add [slack].streaming toggle (send-once mode)#1115dogzzdogzz wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
PR #1115 Code Review — feat(slack): add [slack].streaming toggle (send-once mode)
Related issue: #1114 (RFC)
Branch: dogzzdogzz:feat/slack-streaming-toggle → openabdev:main
Single commit: f249b60
Diff size: 7 files, +70 / -8
1. Overall (TL;DR)
A narrow, backward-compatible, cleanly implemented configuration-flag PR. It adds a streaming switch under [slack], defaulting to true (current behavior). When set to false, the Slack adapter posts a single final message per turn — no native streaming and no post+edit placeholder.
Recommendation: Mergeable once the two RFC open questions (naming, Discord symmetry) are resolved at the design level. Nothing in the implementation blocks merge.
2. Implementation review
2.1 Correct gating points
Both gates are placed at the right spots:
src/slack.rs:537—use_streaming()prefixed withself.streaming &&, controls post+edit placeholdersrc/slack.rs:549—uses_native_streaming()likewise gated, controlschat.startStream
uses_assistant_status() (src/slack.rs:544) is intentionally unaffected, which is the right call — the test assistant_mode_gates_status_and_native_streaming at line 2114 asserts this explicitly. The user still gets the "Thinking…" status set via assistant.threads.setStatus; only the typewriter effect on the final message goes away. That UX trade-off is reasonable.
2.2 Dispatch flow integration
The send-once branch at src/adapter.rs:946 already exists — it's the path Discord falls through to when other_bot_present=true. streaming=false reuses it directly, without introducing a new code path. Risk surface is therefore tiny.
stream_begin (adapter.rs:698) is lazily triggered: only fires on first Text event when native=true. With streaming=false, native is always false, so stream_begin is never called → no placeholder leakage.
2.3 Config structure
src/config.rs:431-441 uses #[serde(default = "default_true")] so existing configs upgrade with zero changes. The doc comment explicitly explains:
Mirrors
[gateway] streamingin concept, but the default deliberately differs:GatewayConfig.streamingdefaults tofalse, whereas this defaults totrueto preserve current Slack streaming.
Good doc comment — anyone confused by the asymmetric defaults between this and GatewayConfig.streaming (config.rs:467) gets the rationale on the spot.
2.4 Test coverage
src/slack.rs:2110-2114 adds a third adapter (streaming=false, assistant_mode=true) to the assistant_mode_gates_status_and_native_streaming test, with three assertions:
assert!(!adapter3.use_streaming(false), "streaming=false forces send-once (no post+edit)");
assert!(!adapter3.uses_native_streaming(false), "streaming=false disables native even with assistant_mode");
assert!(adapter3.uses_assistant_status(), "streaming switch does not affect assistant status API");These cover all three gates' states cleanly — no obvious gap.
2.5 Helm chart
charts/openab/templates/configmap.yaml:141-144 mirrors the rendering pattern used for assistant_mode right above it: only emit to config.toml when explicitly set to false, otherwise defer to the Rust default. This avoids drift between chart-side and Rust-side defaults.
charts/openab/tests/configmap_test.yaml:184-204 adds two symmetric tests verifying "explicit false renders" and "explicit true does not render".
values.yaml:305-307 documents the use case (multi-agent threads avoiding app_mention re-fires), which is genuinely helpful for Helm users.
3. Discussion points
3.1 Naming (RFC open question #1)
The RFC listed three candidates: streaming / live_streaming / send_once. Author chose streaming.
I think streaming is acceptable but not optimal:
- ✅ Symmetric with
[gateway] streaming ⚠️ Could be confused withassistant_mode, which is itself a flavor of streaming⚠️ The semantics aren't quite "enable streaming"; settingstreaming = falseis really "force send-once, overriding everything else"
live_streaming or even send_once (inverted boolean, default false) would be more obvious. But you pay a symmetry cost. A judgment call for the maintainer — not a blocker.
3.2 Discord symmetry (RFC open question #2)
The RFC notes "Discord lacks post+edit streaming", but the Discord adapter does have placeholder + edit (src/discord.rs:135's use_streaming). This PR only touches Slack, without adding a symmetric Discord switch.
I'd lean toward merging the Slack part first (it already addresses the #1103 phantom-turn bug) and tracking Discord symmetry as a follow-up issue, because:
- Discord's
MESSAGE_UPDATEdoesn't deliver mention notifications (already noted inadapter.rs:921), so the phantom-turn risk is lower there - Slack-adapter diff is already non-trivial; bundling Discord would expand the review surface
3.3 Startup log
The current streaming value is not surfaced at startup. There is per-turn debug logging (src/slack.rs:550-555), but it's at debug level and per-turn only.
Suggest adding an info-level startup log near main.rs:236-243 after SlackAdapter::new:
tracing::info!(streaming = s.streaming, assistant_mode = s.assistant_mode, "slack adapter configured");This makes diagnosing "I set streaming = false but still see streaming behavior" a one-line check. Optional polish — non-blocking.
3.4 Documentation consistency
config.toml.example:42 indentation looks slightly off relative to the assistant_mode comment block right above it. Inline format should align. Cosmetic.
3.5 What's missing (checklist)
- ✅ No schema migration needed (additive field)
- ✅ No changelog entry needed (release flow handles it)
- ✅
clippyshould be clean (no new lint risk) ⚠️ No integration test that goes throughconfig.toml→ deserialize →SlackAdapterto verifyuse_streaming()returns the expected value. Current unit tests callSlackAdapter::new(..., false)directly, bypassing the deserialize path. Aconfig.rsround-trip test assertingstreaming = falselands ass.streaming == falsewould be belt-and-braces. Nice-to-have, not required.
4. E2E verification (actually executed — 2026-06-15 07:24–07:27 UTC)
Test environment: compose.issue1114.yaml + openab-claude:pr1115 image, bot1 with streaming = false, bot2 with default. Driven autonomously via agent-browser + xoxc-token in #mcpsupportbot-test (C0AV8B98NKV).
| # | Scenario | Expected | Observed | Pass |
|---|---|---|---|---|
| T1 | @bot1 standalone mention |
One final message, no edits | bot1 log: streaming=false … native=false; reply edited=null, 354 chars in a single send |
✅ |
| T2 | @bot2 standalone mention (default streaming) |
Typewriter / edit loop visible | bot2 log: streaming=true … native=true; reply edited=1781508333.000000, updated ~1.5s later |
✅ |
| T3 | @bot1 mentions @bot2 in a thread |
bot2's app_mention fires exactly once |
bot1 reply edited=null; bot2 receives 1 mentions_bot=true from bot1's reply; bot2's next turn has other_bot_present=true → use_streaming=false, so no edit storm |
✅ |
Additional observations:
- The
slack assistant_mode decisiondebug log correctly surfaces the newstreamingfield — helps when debugging. - During T3, bot2's own streamed reply broadcasts
subtype="message_changed"events to every socket (including its own and bot1's). These events carrymentions_bot=falseso they don't re-trigger dispatch — the adapter handles this correctly. - With
assistant_mode=true+streaming=false, the assistant status API still fires (sinceuses_assistant_status()is not gated by the new switch) — matches both the PR's intent and the unit test assertion. - No panics, no errors, no leftover placeholder ("…") messages throughout the E2E run.
5. Conclusion
Approve — all three E2E scenarios pass, implementation is correct with complete test coverage, fully backward compatible.
Remaining items are design-level (RFC open questions):
- Naming
streamingvslive_streaming/send_once— I lean towardstreamingbeing acceptable-but-ambiguous; maintainer's call. - Discord symmetry — recommend tracking as a follow-up issue (Discord's phantom-turn risk is lower).
- Strong suggestion to fold into this PR: §3.3 startup info log (a single
tracing::info!(streaming = s.streaming, …)). Saves a lot of debugging time.
The E2E run also incidentally verified that the existing other_bot_present=true → use_streaming=false rule from #534 still holds — this PR does not regress prior behavior.
|
Added a follow-up commit ( Why: the turn buffer concatenates every Scope: it's in the shared Correctness: |
|
Follow-up review for What I likeThe motivation is real — agents emit inter-tool narration ("let me pull the diff", "now reading X") that bleeds into send-once replies and makes them read like stream-of-consciousness, while a tool-posted artefact reads like a single composed string. This commit aligns send-once with the artefact form. Implementation is contained:
Test coverage is genuinely thorough: UTF-8 char-boundary fallback, leading directive preserved across tools, no-tool case re-stripping the directive header, streaming preserves full body + directive. The Discussion points1. PR scope/title now extends beyond SlackThe follow-up applies to every send-once turn — not just
Existing users on those platforms will see their messages get noticeably terser after upgrading. Most will probably like it (it's the artefact-form pitch), but it is a silent behavior change. Worth either:
2.
|
| # | Scenario | Expected | Observed | Pass |
|---|---|---|---|---|
| T4 | @bot1 (streaming=false) prompt that requires tool use ("read /etc/hostname then tell me the hostname in one short sentence") |
Only the post-last-tool answer block reaches Slack; no "let me check the file" narration | bot1 log: streaming=false … native=false; reply 72 chars, edited=null: :white_check_mark: `Read /etc/hostname` The hostname is `c006f123b639` . — i.e. tool-line indicator + clean 17-char final answer, zero pre-tool narration |
✅ |
| T5 | @bot1 prompt that triggers no tools ("what is 2+2? answer in one short sentence without using any tools") |
Full reply preserved (regression guard: answer_start == 0 keeps the whole buffer) |
reply 10 chars, edited=null: 2 + 2 = 4. — pure answer, nothing dropped |
✅ |
| T6 | Send-once with leading [[reply_to:<msg_ts>]] directive emitted before a tool |
Directive parsed from full buffer and applied; body has header stripped | Deferred — the 7 unit tests added in this commit cover the directive-preservation logic from every angle I could think of (leading-directive-survives-tools, no-tool case re-strips header, streaming preserves both, UTF-8 char-boundary fallback). I am willing to take the unit tests at face value here rather than engineer an agent prompt that emits [[reply_to:...]] reliably. |
➖ |
Additional notes from the E2E run:
- The
slack assistant_mode decisiondebug log still correctly surfacesstreaming=falseafter the follow-up commit — per-turn decision logging is intact. - No empty replies, no
_(no response)_sentinel fallthrough, no leftover placeholder messages. - Subjective UX read: T4's reply ("The hostname is
c006f123b639.") is meaningfully tighter than the equivalent onf249b60, which would have included a sentence or two of "let me read the file" / "I'll check the hostname" prefacing the answer. The artefact-form pitch in the commit message lands in practice.
Verdict
Direction supported. The behavior change is well-motivated, the implementation is clean, and the tests are good. The items above are clarification asks, not blockers — the only one I'd push for in this PR is either a PR title/release-note adjustment for the broader scope, or a one-liner in the select_delivery_text doc comment stating the "answer follows last tool" invariant.
github-actions
Bot
added
needs-rebase
pending-contributor
and removed
pending-maintainer
labels
dogzzdogzz
force-pushed
the
feat/slack-streaming-toggle
branch
from
0868f18 to
9f55a29
Compare
github-actions
Bot
added
pending-contributor
and removed
needs-rebase
pending-contributor
labels
This comment has been minimized.
This comment has been minimized.
Final Aggregated Review — PR #1115
Modegroup-review (3/3 voices active: Claude + Codex + Gemini) · R1 independent + R2 cross-debate TL;DRThe core Rust logic is correct (UTF-8 boundary handling, Consensus Important (should fix before merge)1.
|
github-actions
Bot
added
the
closing-soon
openab-app
Bot
removed
the
closing-soon
github-actions
Bot
added
the
closing-soon
This comment has been minimized.
This comment has been minimized.
antigenius0910
left a comment
antigenius0910
left a comment
There was a problem hiding this comment.
Approving
Direction supported in my Jun 16 follow-up, and nothing in the subsequent rebases (#1136 / #1137 / #1138 / #1139 / #1145) changes that read — they're orthogonal to send-once. The behavior change is well-motivated, the implementation is clean, the tests are good.
Optional contribution — unit test for the reset re-prepend branch
In follow-up review item #3 I called out that the inline branch at the end of stream_prompt_blocks —
let text_buf = if reset && !keep_full_text && answer_start > 0 {
format!("⚠️ _Session expired, starting fresh..._\n\n{text_buf}")
} else {
text_buf
};— wasn't exercised by a unit test even though it's the exact edge case that would silently drop the session-reset notice on a tool-using turn. I built a 4-corner truth-table test for it locally, commit b07d295 on test/PR1115-add-finalize_body-test (in my worktree):
- Extracts the inline if-else into a pure helper
pub fn finalize_body(reset, keep_full_text, answer_start, body) -> String. - No behavior change — same truth table, same call site.
- 4 tests covering every corner of
(reset, keep_full_text, answer_start > 0):reset + send-once + tool ran→ notice re-prependedreset + send-once + no tools→ pass through (notice already in slice)reset + keep_full_text→ pass throughno reset(both flag combos) → pass through
cargo test passes on top of your 78fdee2. Embedding as a diff block below — git apply directly if you want it, ignore otherwise. I'm not pushing the branch (PAT scope mismatch with the workflow files pulled in by the rebases; not worth a token swap for one test commit).
git apply-able diff (124 lines, 1 file)
diff --git a/src/adapter.rs b/src/adapter.rs
index c95ad23..e0ca226 100644
--- a/src/adapter.rs
+++ b/src/adapter.rs
@@ -164,6 +164,33 @@ pub fn split_delivery(
(directives, body)
}
+/// Apply the session-reset re-prepend rule to a finalized turn body.
+///
+/// The session-reset notice (`"⚠️ _Session expired, starting fresh..._\n\n"`)
+/// is pushed at the head of the turn buffer so streaming consumers see it
+/// live. When the turn ends in send-once trimming mode (`!keep_full_text`) and
+/// a tool ran (`answer_start > 0`), the slice that `select_delivery_text`
+/// returns starts *after* the notice — so re-prepend it to keep the user
+/// aware their session was reset. In every other corner (no reset, or
+/// `keep_full_text`, or no tools ran) the notice is either absent or already
+/// included in `body`, and we must not duplicate it.
+///
+/// Pure helper: deliberately mirrors the inline branch at the end of
+/// `AdapterRouter::stream_prompt_blocks` so the four-corner truth table can be
+/// exercised in isolation without a live ACP session.
+pub fn finalize_body(
+ reset: bool,
+ keep_full_text: bool,
+ answer_start: usize,
+ body: String,
+) -> String {
+ if reset && !keep_full_text && answer_start > 0 {
+ format!("⚠️ _Session expired, starting fresh..._\n\n{body}")
+ } else {
+ body
+ }
+}
+
// --- Platform-agnostic types ---
/// Identifies a channel or thread across platforms.
@@ -1001,12 +1028,9 @@ impl AdapterRouter {
// The session-reset notice lives at the head of the buffer; a
// tool advancing answer_start past it would drop it from the
// slice, so re-prepend it to the (directive-stripped) body in
- // exactly that case (answer_start == 0 keeps it via the slice).
- let text_buf = if reset && !keep_full_text && answer_start > 0 {
- format!("⚠️ _Session expired, starting fresh..._\n\n{text_buf}")
- } else {
- text_buf
- };
+ // exactly that case. `finalize_body` is the pure helper that
+ // encodes the four-corner truth table so it can be unit-tested.
+ let text_buf = finalize_body(reset, keep_full_text, answer_start, text_buf);
// Build final content
let final_content =
@@ -1605,6 +1629,69 @@ mod tests {
assert_eq!(body, "narration then answer");
}
+ // --- finalize_body: four-corner truth table for the reset re-prepend ---
+ //
+ // The send-once trimming logic in `stream_prompt_blocks` ends with an
+ // inline branch that decides whether to re-prepend the session-reset
+ // notice. Extracted into the pure helper `finalize_body` so each corner
+ // of (reset, keep_full_text, answer_start) can be exercised without a live
+ // ACP session. Mirrors the integration-level concern raised in PR #1115
+ // peer review (howie group-review, "Important #3").
+
+ #[test]
+ fn finalize_body_reset_send_once_with_tools_prepends_notice() {
+ // Reset turn, send-once trimming, a tool advanced answer_start past
+ // the notice → the slice no longer contains it → re-prepend.
+ let body = "the final answer".to_string();
+ let out = finalize_body(true, false, 42, body);
+ assert_eq!(
+ out, "⚠️ _Session expired, starting fresh..._\n\nthe final answer",
+ "send-once + reset + tool ran → notice must be re-prepended"
+ );
+ }
+
+ #[test]
+ fn finalize_body_reset_send_once_no_tools_passes_through() {
+ // answer_start == 0 means the slice still equals the full buffer,
+ // which already starts with the notice → re-prepending would
+ // duplicate it.
+ let body = "⚠️ _Session expired, starting fresh..._\n\nthe final answer".to_string();
+ let out = finalize_body(true, false, 0, body.clone());
+ assert_eq!(
+ out, body,
+ "send-once + reset + no tools → body already carries notice, pass through"
+ );
+ }
+
+ #[test]
+ fn finalize_body_reset_keep_full_passes_through() {
+ // keep_full_text means the slice is the whole buffer (incl. the
+ // notice) → must not duplicate, regardless of answer_start.
+ let body = "⚠️ _Session expired, starting fresh..._\n\nnarration then answer".to_string();
+ let out = finalize_body(true, true, 42, body.clone());
+ assert_eq!(
+ out, body,
+ "keep_full_text → body already carries notice, pass through even with tools"
+ );
+ }
+
+ #[test]
+ fn finalize_body_no_reset_passes_through() {
+ // Non-reset turn: there is no notice to manage, irrespective of the
+ // other two flags. Covers both halves of the cartesian product.
+ let body = "the final answer".to_string();
+ assert_eq!(
+ finalize_body(false, false, 42, body.clone()),
+ body,
+ "no reset → never prepend (send-once + tools)"
+ );
+ assert_eq!(
+ finalize_body(false, true, 0, body.clone()),
+ body,
+ "no reset → never prepend (keep_full + no tools)"
+ );
+ }
+
/// Compile-time regression guard: use_streaming() is a required trait methodLGTM either way — approving on the PR itself, the diff above is just a take-it-or-leave-it contribution to close the test gap from my prior review item #3.
antigenius0910
left a comment
antigenius0910
left a comment
There was a problem hiding this comment.
Approving
Direction supported in my Jun 16 follow-up, and nothing in the subsequent rebases (#1136 / #1137 / #1138 / #1139 / #1145) changes that read — they're orthogonal to send-once. The behavior change is well-motivated, the implementation is clean, the tests are good.
Optional follow-up: I opened dogzzdogzz#4 against feat/slack-streaming-toggle adding a unit test for the reset && !keep_full_text && answer_start > 0 re-prepend branch — that's the test gap I flagged as item #3 in my follow-up review. Merge it into your feature branch and it flows into this PR automatically, or skip it entirely — it's a nit, not a blocker for approval.
openab-app
Bot
added
pending-maintainer-review
and removed
pending-community-review
labels
thepagent
added
needs-rebase
and removed
closing-soon
github-actions
Bot
added
the
closing-soon
openab-app
Bot
added
pending-maintainer-review
and removed
closing-soon
Resolution of aggregated review itemsThanks @howie for the thorough review. All items addressed — summary below. #1 — `gateway.streaming` documented but never rendered → removed from docsChose Option A: removed the newly-added docs rather than wiring the Helm template. The Rust field `GatewayConfig.streaming` is pre-existing and functional — the gap (template never renders it) predates this PR. Adding docs for a knob that silently does nothing in Helm was the bug; removing them restores the pre-PR state. The Rust field itself is untouched. Commits: `e1e4a243` (values.yaml, config.toml.example) #2 — `split_delivery` docstring overclaims directive survival → qualified to non-reset turnsDocstring updated to explicitly scope the guarantee: directives survive in normal (non-reset) turns; reset-turn behaviour noted as a pre-existing gap where the session-notice prefix prevents clean directive parsing. Commit: `efbc799` #3 — `stream_prompt_blocks` reset re-prepend untested → `finalize_body` helper + 4 unit testsExtracted the inline 4-line reset re-prepend branch into `pub(crate) fn finalize_body(reset, keep_full_text, answer_start, body)` and added four tests covering every corner of the `(reset, keep_full_text, answer_start > 0)` truth table (reviewed by the agent panel; no logic bugs found). Commit: `67ea868` (merge of PR #4 authored by @antigenius0910) Disputed — send-once default behavior change → documented; maintainer decision requestedKept `narration_display` defaulting `false` (the feature's intent: clean final-answer artefact, not stream-of-consciousness scratchpad). Added a prominent ` Awaiting maintainer sign-off on the opt-out default. #4 — `split_delivery` redundant + unsafe re-parse → fixed`parse_output_directives` is now only called on the delivered slice when `answer_start == 0` (delivered slice starts at the buffer head, where directives live). For later slices (answer-start > 0) the body is used as-is, preventing a final-answer block that opens with `[[…]]` from having its content silently stripped. Commit: `efbc799` #5 — `select_delivery_text` silent fallback → warn log added`unwrap_or(full)` → `unwrap_or_else(|| { tracing::warn!(answer_start, full_len, "stale answer_start offset; delivering full buffer"); full })`. The invariant still holds today; the warn makes a future regression observable. Commit: `efbc799` #6 — Helm negative test matchers too loose → tightened`notMatchRegex: 'streaming'` → `'streaming = '` Commit: `efbc799` Current branch tip: `67ea868` |
Rebased 8 feature commits cleanly onto upstream/main (15 new commits including Cargo workspace restructure openabdev#1146, slack allow-list refactor openabdev#1152, adapter fix openabdev#1153). Auto-merged into crates/openab-core/ new layout. All tests pass, clippy clean.
dogzzdogzz
force-pushed
the
feat/slack-streaming-toggle
branch
from
67ea868 to
c516310
Compare
|
LGTM ✅ — All aggregated review findings addressed; code is correct and well-tested. What This PR DoesAdds a How It Works
Findings
What's Good (🟢)
Baseline Check
Addressing External Reviewer Feedback@howie (Aggregated Review — Round 2)
✅ Addressed in
✅ Addressed in
✅ Addressed in
✅ Addressed in
✅ Addressed in
✅ Addressed in @antigenius0910 (Round 1 + Approval)✅ Approved: Direction supported; no concerns on subsequent rebases.
|
github-actions
Bot
added
the
closing-soon
Question on AGENTS.md §1 compliance for the
|
|
Hi @thepagent — gentle ping for CODEOWNER review when you have a moment. State of the PR:
One outstanding discussion item worth your eye before merge: @howie's §1 backward-compat question (above, 06:41 UTC) about the Thanks! |
5 participants
What problem does this solve?
The Slack adapter always streams replies live — native streaming (
chat.startStream+assistant.threads.setStatus) inassistant_mode(default), or a post+edit placeholder otherwise. Great UX for a single bot, but in multi-agent threads it drives a phantom-turn storm:@-mentions bot B)At step 2 no other bot has posted in the thread yet, so bot A still treats it as a single-bot thread and streams its reply via post+edit. Each edit emits a
message_changedevent → bot B'sapp_mentionre-fires once per edit, so bot B spawns a full agent turn for each intermediate/partial state instead of bot A's single settled message (observed: 2 real messages → 5 turns, one acting on a mid-stream fragment<@U…> -).The existing
other_bot_presentgate only disables streaming after another bot has posted — it cannot help on the kickoff message that first mentions bot B. There is no operator switch to deterministically force send-once.Closes #1114. Addresses the phantom-turn bug #1103 — chosen over the #1104
app_mentiondebounce approach: posting one final message removes the re-fire at the source rather than dedup'ing after.Discord Discussion URL: https://discord.com/channels/1491295327620169908/1491969620754567270/1515891969401032715
At a Glance
Proposed Solution
Add one optional boolean to
[slack], defaulttrue(preserves current behaviour):When
streaming = false:chat.startStream/ streamedassistant.threads.setStatus).chat.postMessageper turn.assistant_mode— the assistant status API (set-status / reaction) is unaffected; only the message streaming path is gated.Gating is a single AND-in at the two decision points:
use_streaming()→self.streaming && !other_bot_presentuses_native_streaming()→self.streaming && self.assistant_mode && !other_bot_presentValidation
streaming = true(default): identical behaviour to today — native streaming in single-botassistant_modethreads, post+edit otherwise,other_bot_presentstill disables streaming when another bot has posted.streaming = false:use_streaming()anduses_native_streaming()both returnfalseeven when alone;uses_assistant_status()is unaffected (status API independent of the streaming switch).Testing
cargo test --bin openab— passing, incl. extendedassistant_mode_gates_status_and_native_streamingcases assertingstreaming=falseforces send-once (no post+edit, no native) while leaving assistant status intact.cargo clippy --all-targets -- -D warnings— clean.charts/openab/tests/configmap_test.yamlcoversstreamingomitted (default, key absent) andstreaming: false(rendersstreaming = false).Update (2026-06-18) — narration trimming via a dedicated
narration_displayflagFollow-up commit
733abf5added "send-once delivers only the final answer block" — dropping the inter-tool narration ("let me check… / now reading…") that otherwise leaks into the message. Per maintainer discussion this is being decoupled fromstreaminginto its own flag rather than implicitly coupled to send-once:narration_display: bool, defaultfalse, on the adapter config.narration_display = true. (Streaming shows narration live regardless, so the flag only bites in send-once.)default falsepreserves733abf5's behaviour (trim by default); settrueto keep the full inter-tool text.streaming=false, Slack/Discord multi-bot threads, and gateway platforms — not just Slack. This supersedes the per-backend narration filters in fix(agy-acp): filter narration based on OPENAB_TOOL_DISPLAY #1030 (agy-acp) and feat(acp): filter opencode planning narration from responses #1032 (opencode).This also resolves the "is
streamingoverloaded?" review point — two independent switches:streaming= stream vs send-once;narration_display= include inter-tool narration or only the final block.narration_displaydefaultsfalse, which means all existing send-once paths deliver only the final answer block after upgrade — inter-tool narration is silently dropped. Affected paths:GatewayConfig.streamingdefaultsfalse, so every gateway turn was already send-once.other_bot_present=truewere already send-once.streaming=false(new in this PR) — obviously affected, but this is opt-in so no existing deployment is surprised here.To restore the old full-text behaviour, set
narration_display = truein[reactions]:The rationale for defaulting to
false: send-once messages read like a composed artefact (clean, direct answer) rather than a stream-of-consciousness scratchpad. Most operators on gateway/multi-bot platforms will prefer this. Operators who want the full narration can opt back in.Update (2026-06-21) — review fixes
Addressed remaining findings from the aggregated review:
gateway.streamingdocs removed —GatewayConfig.streamingis a pre-existing Rust field whose Helm template rendering was never wired (pre-existing gap). This PR was advertising it invalues.yaml/config.toml.examplewithout wiring the Helm template, creating a silent no-op. Docs removed; the Helm gap will be tracked separately.split_deliveryre-parse conditioned — the secondparse_output_directivescall now only runs when the delivered slice begins at byte 0 (answer_start==0orkeep_full). Whenanswer_start>0the slice is mid-buffer reply text; re-parsing it could wrongly strip a final answer that opens with[[...]].split_deliverydocstring qualified — directive-preservation guarantee now explicitly scoped to non-reset turns.select_delivery_textstale-offset warning —unwrap_orreplaced withunwrap_or_elsethat emits atracing::warn!so a future regression is observable rather than silently leaking narration.notMatchRegex: 'streaming'→'streaming = 'andnotMatchRegex: 'narration_display'→'narration_display = 'to avoid matching comments or unrelated key names.