feat(ambient): observe threads under configured channels (default-on) by chaodu-agent · Pull Request #1220 · openabdev/openab

chaodu-agent · 2026-06-27T12:55:34Z

What problem does this solve?

Ambient v1 only buffered top-level messages in a configured channel — routing in discord.rs was gated on !in_thread. But OpenAB auto-creates a thread per conversation, so ~99% of activity is in threads and ambient observed almost nothing (verified live on chaodu).

Change

When ambient is enabled, messages in a thread whose parent is a configured channel are observed by default — no extra config knob (ambient's existing enabled flag is the opt-in; once on, it covers the channel and its threads).

Per-thread batching — keyed by the thread's channel_id; replies post back into that thread.
All threads observed — both bot-owned and non-owned threads are passively observed. The bot can follow conversation in threads it created without requiring an @mention (acting as thread lead). Previously, bot-owned threads were excluded, which meant the bot could not see replies unless explicitly mentioned.
@mention priority preserved — a mention discards the buffer and falls through to immediate dispatch, preventing double-reply.

Routing is encapsulated in a pure, unit-tested method:

AmbientDispatcher::should_buffer(channel_id, in_thread, bot_owns_thread, parent_id) -> bool

Compatibility

No new config field. Behavior only changes for deployments that already have ambient enabled — those now also observe threads under their configured channels (the intended behavior, since channel-only observation was nearly useless given OpenAB's thread-centric UX).

Tests

should_buffer unit tests: top-level match, thread-under-ambient-channel (both owned and non-owned → buffered), parent-not-ambient negative, no-parent negative, and disabled-when-ambient-off.

Docs

docs/ambient.md updated: threads-observed-by-default note + both owned/non-owned threads observed + per-thread-batching semantics.

Ambient v1 only buffered top-level messages in a configured channel (gated on !in_thread). But most OpenAB conversation happens in auto-created threads, so ambient saw almost nothing. Now, when ambient is enabled, messages in a thread whose parent is an ambient channel are observed by default — keyed by the thread id so each thread batches independently. Only threads the bot does NOT own are observed passively; threads the bot owns keep using normal (immediate) dispatch, so there's no double-handling. @mention still discards the buffer and takes priority. No new config knob — ambient's existing 'enabled' flag is the opt-in; once on, it covers the channel and its threads. Routing is encapsulated in AmbientDispatcher::should_buffer(channel_id, in_thread, bot_owns_thread, parent_id) with unit tests.

Remove the !bot_owns_thread gate from should_buffer(). Bot-owned threads are now also passively observed via ambient — the bot can follow thread conversation it started without requiring @mention. An @mention still discards the buffer and triggers immediate dispatch.

Z渡 correctly identified that ambient thread observation was unreachable when the channel was only in [ambient.discord].channels but not in [discord].allowed_channels. The early return gate and detect_thread() both depended on the normal allowlist. Fix: - Always preserve thread_parent_id (not gated on detect_thread result) - Add is_structural_thread from thread_metadata for ambient routing - Compute in_ambient_context before the early return gate - Let ambient-eligible messages bypass the normal allowlist gate Also addresses reviewer suggestions: - Add startup info! log when ambient+threads is active - Add tracing::debug! on thread_parent_id parse failure

Unconditionally setting thread_parent_id to gc.parent_id caused non-thread channels (under categories) to incorrectly populate SenderContext with the category ID as parent. Fix: split into two fields: - thread_parent_id (String): gated on has_thread_metadata, used for SenderContext and downstream - structural_parent_id (u64): gated on has_thread_metadata, used for ambient routing (avoids string parsing entirely)

chaodu-agent · 2026-06-27T13:38:19Z

LGTM ✅ — All findings from previous review rounds addressed; thread observation under ambient channels is clean and ready to merge.

What This PR Does

Ambient mode v1 only buffered top-level channel messages (!in_thread gate), missing ~99% of OpenAB activity that happens in auto-created threads. This PR makes ambient observe all threads under configured channels by default, with per-thread batching and preserved @mention priority.

How It Works

New should_buffer() method — pure decision function on AmbientDispatcher: top-level message in ambient channel → buffer; thread whose parent is ambient channel → buffer (regardless of bot ownership).
is_structural_thread — decouples raw thread detection (thread_metadata.is_some()) from the dispatch-allowlist-filtered in_thread, enabling ambient routing to work independently of allowed_channels.
thread_parent_id correctly gated — only set when has_thread_metadata is true, preventing category IDs from leaking into SenderContext for non-thread messages.
Early-return gate updated — adds !in_ambient_context so ambient-only channels pass through.
@mention priority — mention in ambient context discards buffer → immediate dispatch (no double-reply).
Per-thread batching — each thread buffers independently keyed by its channel_id.

Findings

#	Severity	Finding	Location
1	🟢	`thread_parent_id` correctly gated on `has_thread_metadata` — previous critical regression fixed	`discord.rs:583`
2	🟢	`is_structural_thread` decoupling is the right design — ambient routing independent of dispatch allowlist	`discord.rs:554-586`
3	🟢	`in_ambient_context` check before early return gate ensures ambient-only channels work	`discord.rs:610-618`
4	🟢	`should_buffer` is pure, well-documented, trivially testable	`ambient.rs:298-327`
5	🟢	Comprehensive unit tests: 3 test functions, 7 assertions covering all branch combinations	`ambient.rs:624-667`
6	🟢	Startup `info!` log gives operators visibility into thread-observation scope expansion	`ambient.rs:276-280`
7	🟢	`unwrap()` on `self.ambient` is safe — guarded by `in_ambient_context` which requires `Some`	`discord.rs:628`
8	🟢	Docs clearly distinguish `allowed_channels` vs `ambient.discord.channels` with OR relationship table	`docs/ambient.md`
9	🟢	Config comments synced across all doc files to mention thread observation	`docs/config-reference.md`, `docs/discord.md`

Addressing Previous Review Findings

🔴 (Round 1) — `thread_parent_id` unconditionally set corrupts `SenderContext`

✅ Fixed: Now gated on has_thread_metadata — category IDs no longer leak for non-thread guild messages.

🟡 (Round 1) — Config comment consistency

✅ Addressed: All three doc files now mention "and their threads".

🟡 (Round 1) — Startup log for scope expansion

✅ Addressed: tracing::info! with channel list added at dispatcher construction.

Baseline Check

PR opened: 2026-06-27
Main already has: AmbientDispatcher with is_ambient_channel(), top-level-only routing gated on !in_thread, per-channel batching
Net-new value: Thread observation under ambient channels (both bot-owned and non-owned), should_buffer() pure decision method, is_structural_thread decoupled from allowlist, startup log, comprehensive docs update

What's Good (🟢)

Pure function design — should_buffer has no I/O, no async, no side effects
Smart decoupling of is_structural_thread from in_thread — correct architectural choice
Minimal blast radius: only affects deployments with ambient already enabled
Forward-compatible _bot_owns_thread parameter reserved for future per-thread policy
No new config knobs — leverages existing enabled flag
Excellent documentation with comparison table between the two independent allowlists
@mention behavior in ambient-only channels explicitly documented

⚠️ CI status: Checks still pending at review time. Approval conditional on CI passing.

chaodu-agent requested a review from thepagent as a code owner June 27, 2026 12:55