docs: add ADR for turn-boundary message batching#598
docs: add ADR for turn-boundary message batching#598thepagent merged 4 commits intoopenabdev:mainfrom
Conversation
Records the structural decision extracted from RFC openabdev#580 (Turn-boundary message batching): how N concatenated arrival events are packed into the Vec<ContentBlock> crossing the broker → ACP boundary. Reuses the existing per-arrival-event <sender_context> template repeated N times rather than introducing a parallel <message index=N> wrapper schema, with one additive `timestamp` field on SenderContext. Closes the attribution gap surfaced independently by Triage (T1.4) and JARVIS/FRIDAY (B1) reviews.
github-actions
Bot
added
closing-soon
|
request input from @masami-agent and @shaun-agent |
masami-agent
left a comment
masami-agent
left a comment
There was a problem hiding this comment.
PR Review: #598
Summary
- Problem: RFC #580 MVP packing flattens
extra_blocksto a tail, destroying attachment ↔ message attribution (flagged independently by T1.4 and B1). - Approach: Reuse existing
<sender_context>template repeated N times with one additivetimestampfield — no new schema, no wrapper tags. - Risk level: Low (docs-only ADR, no code change in this PR)
Core Assessment
- Problem clearly stated: ✅ — grounded in two independent reviews (T1.4, B1) that converged on the same gap
- Approach appropriate: ✅ — reuses existing
<sender_context>rather than inventing parallel schema; minimal surface area - Alternatives considered: ✅ — six alternatives (§6.1–6.5) each with clear rejection rationale
- Best approach for now: ✅ — single uniform code path, no ACP protocol change, additive schema evolution
Findings
Verified against current codebase:
SenderContextstruct inadapter.rscurrently has:schema,sender_id,sender_name,display_name,channel,channel_id,thread_id(optional),is_bot— notimestampfield, confirming the ADR's claim that the addition is purely additive ✅handle_messageinadapter.rsdoes prepend Textextra_blocks(transcripts) before<sender_context>+prompt, then appends non-Text blocks (images) after — confirming the ADR's description of the current asymmetric ordering ✅- The
prompt_with_senderformat matches:<sender_context>\n{json}\n</sender_context>\n\n{prompt}✅ - Sibling ADR links (
./multi-platform-adapters.md,./custom-gateway.md) both resolve ✅ - Tracking issue #580 exists, is OPEN, and its comments confirm T1.4 and B1 independently flagged the attribution gap ✅
- ADR filename follows existing kebab-case convention ✅
What I especially liked:
- The Highest Guideline (§2) with five concrete prohibitions gives future implementation PRs a clear test surface — any packing change can be judged against these rules
- §8 Compliance section is unusually thorough — 8 rules + explicit prohibited transformations in §8.1 make the ADR self-enforcing
- Honest about negatives: Scenario D regression and token cost are acknowledged, not hidden
- The rollback path for Scenario D is pragmatic — hotfix PR rather than permanent feature flag
Review Summary
🔧 Suggested Changes
- §3.1
thread_idomission: TheSenderContextstruct has#[serde(skip_serializing_if = "Option::is_none")]onthread_id, so it's absent from serialized JSON whenNone. The example JSON in §3.1 omits it without explanation. Consider either including it in the example (e.g."thread_id": "123456") or adding a note like (fields withskip_serializing_ifbehavior, such asthread_id, are omitted whenNone) so readers don't wonder if it was forgotten. - §3.2 backward compatibility note: The ADR says "schema stays
openab.sender.v1" — worth adding one sentence explicitly noting that consumers using lenient JSON parsing (ignoring unknown fields) won't break, since this is the property that makes the change safe. Something like: "Consumers that ignore unknown JSON fields (the default forserdewith#[serde(deny_unknown_fields)]absent) will continue to work unchanged."
⚪ Nits
- Stale line numbers: References to
adapter.rs:131-152,138-143,138-152,148-152are ~20 lines off from the current code (the logic moved down due to added comments and thethread_idfield). Not blocking since the logic descriptions are accurate, but updating them would prevent confusion for readers checking the source. - §5 Scenario B visual gap: The example shows a blank line between the two
<sender_context>blocks, but in the actualVec<ContentBlock>array these would be separateContentBlockentries with no visual gap. Minor presentation issue — the structural meaning is clear regardless.
Verdict
APPROVE ✅
This is an exceptionally well-written ADR. The design decision is sound — reusing <sender_context> with adjacency-based attribution is simpler and more robust than the RFC MVP's wrapper-and-flatten approach. The compliance section and prohibited transformations list will serve as a durable reference for implementation PRs. The suggested changes above are non-blocking improvements.
Follow-up Suggestions (post-approval)After discussing this ADR in more depth, I have three suggestions for the implementation phase. These are not blockers for merging this ADR — the design direction is sound and should land as-is. These are guardrails for RFC #580 Phase 1. 1. Treat this ADR as a living document during Phase 1This ADR is thorough (350 lines, 8 compliance rules, 5 prohibitions), but the implementation PR for RFC #580 has not been opened yet. Real code has a way of surfacing edge cases that design docs miss. Suggestion: When Phase 1 implementation begins, if the implementer discovers a case the ADR did not anticipate, a follow-up PR to amend the ADR should be welcomed — not treated as a violation. The compliance rules in §8 are valuable as guardrails, but they should evolve with implementation experience rather than become immutable before any code is written. 2. Make observability metrics a Phase 1 must-haveThe ADR defines three metrics for monitoring token cost growth (§7 Negative consequences):
And a re-evaluation threshold (p95 × envelope tokens > 500 per dispatch). Without these metrics actually being implemented, the threshold will never trigger and the dedup discussion will never happen — even if token costs become a real problem in production. Suggestion: Track these three metrics as blocking items in the RFC #580 Phase 1 implementation PR, not as a Phase 2 follow-up. 3. Define Scenario D smoke test criteria before Phase 1The ADR acknowledges a behavior change for voice-only messages (STT transcripts move from before Suggestion: Before the Phase 1 implementation PR is opened, define a minimal smoke test matrix:
This ensures the rollback decision is based on concrete evidence, not subjective judgment. These three items could be tracked as acceptance criteria on RFC #580, or as a checklist in the Phase 1 implementation PR description. |
obrutjack
left a comment
obrutjack
left a comment
There was a problem hiding this comment.
PR Review: #598
Summary
- Problem: RFC #580 MVP packing flattens
extra_blocksto a tail, destroying attachment ↔ message attribution (T1.4 / B1). - Approach: Reuse existing
<sender_context>template with additivetimestampfield; attachments attributed by array adjacency. - Risk level: Low (docs-only PR, no runtime changes)
Core Assessment
- Problem clearly stated: ✅
- Approach appropriate: ✅
- Alternatives considered: ✅ (§6 covers 5 alternatives with clear rejection rationale)
- Best approach for now: ✅
Source Code Verification
Verified against current main:
adapter.rsSenderContextstruct confirms notimestampfield — additive change is accurateadapter.rs:handle_messageconfirms transcript Text blocks prepended, image blocks appended — §3.5 / Scenario D behavior change description is accuratediscord.rsconfirms voice transcript usesextra_blocks.insert(0, ...)— consistent with ADR- Sibling ADR links (
multi-platform-adapters.md,custom-gateway.md) resolve ✅ - RFC #580 tracking reference resolves ✅
Review Summary
💬 Questions
- §3.2 — Gateway adapter uses
chrono::Utc::now().to_rfc3339()as best-effort timestamp. Does the current gateway inbound event schema (openab.gateway.event.v1) already carry a timestamp? If so, the ADR should note that the gateway adapter should prefer the event's own timestamp over broker receive time.
🔧 Suggested Changes
- §8 Compliance rule 1 lists "broker expanding Discord
<@123>mentions to@usernamestrings" as a prohibited transformation, butdiscord.rs'sresolve_mentions()already does this. Suggest either removing this counter-example or adding a caveat that mention resolution is an adapter-layer transformation (before{prompt}enters the broker), not subject to this rule. - §7 Negative — the observation threshold formula (
p95_batch_size × context_tokens_per_event > 500 tokens) mixes count and token units. Consider clarifying this is the per-dispatch envelope overhead in tokens.
⚪ Nits
- §3.2 —
slack_ts_to_iso8601(event.ts)doesn't exist in the current codebase. Consider marking it as a proposed helper to avoid confusion.
Verdict
COMMENT — requesting contributor response on the resolve_mentions contradiction (Suggested Change #1) before maintainer approval. No blockers.
Replaces the MVP wrapper-and-flatten packing (banner + <message index=N> tags + flattened extra_blocks tail) with the repeated-<sender_context> design recorded in docs/adr/batched-turn-packing.md. Each buffered arrival event is packed independently via the new pack_arrival_event helper; attachments interleave in arrival order so attribution is recoverable by ContentBlock array adjacency, closing the T1.4 / B1 attribution gap. - adapter: add SenderContext.timestamp (additive — schema stays openab.sender.v1); add pack_arrival_event helper and AdapterRouter::dispatch_batch entry point that skips the legacy single-<sender_context> wrapping. - dispatch: src/dispatch/mod.rs → src/dispatch.rs. Consumer drains the per-thread mpsc and concatenates pack_arrival_event(...) per event; no banner, no <message> wrapper, no XML escaping, no per-batch sender merge. Reactions still anchor on the trailing message's trigger_msg. Backpressure stays at park-on-full (tx.send().await) — ADR §2.1 rule 4 forbids silent drop. - discord: populate timestamp from msg.timestamp (serenity Display → RFC 3339); drop sender_name from BufferedMessage (no longer needed now that each event keeps its own sender_json). - slack: populate timestamp via slack_ts_to_iso8601 (epoch.fraction → ISO 8601 ms); chrono added as direct dep. - gateway: thread event.timestamp into SenderContext (best-effort, receive time per ADR §3.2). - config: default max_buffered_messages 10 → 30 (ADR §7 phase-1 cap); MessageProcessingMode flag retained for staged rollout. Tests: unit tests for pack_arrival_event in adapter.rs and for the batched concatenation shape in dispatch.rs. cargo check not run locally (orchestrator container lacks a C linker) — verify on a dev machine. Co-Authored-By: Claude Opus 4.7 <[email protected]>
Replaces the standalone packing ADR with the consolidated turn-boundary message batching ADR, which folds RFC openabdev#580 mechanism, T1.x dispositions, and the original packing design into a single document. Co-Authored-By: Claude Opus 4.7 <[email protected]>
brettchien
changed the title
|
Updated this PR to expand the ADR's scope. The original The original packing decision is preserved as §3 of the consolidated ADR; the additional sections cover the mechanism (§2), config & rollout (§4), alternatives (§5), and consequences/compliance (§6). |
masami-agent
left a comment
masami-agent
left a comment
There was a problem hiding this comment.
PR Re-Review: #598 (v0.3 — consolidated ADR)
Context
My previous APPROVE was based on the original batched-turn-packing.md (packing-only scope). The contributor has since replaced it with a comprehensive turn-boundary-batching.md covering mechanism, packing, config/rollout, alternatives, and compliance — a substantially different document at 1051 lines. This is a fresh review of the v0.3 content.
Summary
- Problem: Within one thread, messages arriving during an in-flight ACP turn become independent sequential turns, wasting intermediate output and losing attachment attribution.
- Approach: Per-thread bounded
mpsc::channel+ consumer task; greedy drain at turn boundaries; N repetitions of existing<sender_context>template as packing format. - Risk level: Low (docs-only ADR, no code change)
Core Assessment
- Problem clearly stated: ✅ — three concrete workload patterns (§1.1), grounded in current code paths
- Approach appropriate: ✅ — three invariants (I1/I2/I3) are well-defined and load-bearing for the rest of the document
- Alternatives considered: ✅ — six mechanism alternatives (§5.1) + four packing alternatives (§5.2) + prior art comparison (§5.3), each with clear rejection rationale
- Best approach for now: ✅ — turns an architectural constraint (no mid-turn interrupt for external ACP CLIs) into a feature
What I especially liked
- §2.5 race-safe eviction — the
generation: u64counter with double-observer analysis is thorough. The explicit enumeration of what happens when two concurrentsubmitcalls observeSendErroron the same handle is exactly the kind of detail that prevents subtle bugs in the implementation PR. - §2.7 honest scoping — acknowledging the zombie blast radius widens under batching (axis 2) without trying to fix it in this ADR is the right call. The two-axis framing makes the risk concrete.
- §3.4 three-way comparison table — makes the design delta crystal clear vs. current code and RFC MVP.
- §5.3 prior art — source-level comparison with Hermes and OpenClaw, including the architectural constraint analysis (in-process vs external ACP CLI), is unusually rigorous for an ADR.
- §6.4 + §6.5 compliance rules + prohibited transformations — these will serve as a durable test surface for future PRs. The explicit "categorically forbidden" list in §6.5 prevents re-litigation.
- Appendix A — the reference implementation sketch is directly usable as a Phase 1 starting point.
Findings
💬 Questions (re-raised from previous review cycle)
-
§6.4 rule 1 —
resolve_mentionsscope clarification needed.Rule 1 lists this as a prohibited counter-example:
"broker expanding Discord
<@123>mentions to@usernamestrings"I verified the current
resolve_mentions()indiscord.rs:1068-1077— it does not expand<@123>to@username. It only: (a) strips the bot's own trigger mention, and (b) replaces role mentions with@(role). User mentions are preserved as raw<@UID>.So the counter-example describes a transformation that does not currently exist — which is fine as a prohibition. However,
resolve_mentions()does perform two transformations on{prompt}content before it reaches the broker:- Stripping
<@bot_id>(the trigger mention) - Replacing
<@&role_id>with@(role)
These are adapter-layer transformations that happen before
{prompt}enters the packing pipeline. The ADR should clarify that rule 1 applies to the broker/dispatcher layer, not to adapter-level preprocessing — otherwise a reader checking compliance could flagresolve_mentions()as a violation.This was raised in @obrutjack's previous review (Suggested Change #1) and has not been addressed in v0.3. Requesting a response.
- Stripping
🔧 Suggested Changes
-
§6.4 rule 1 — add adapter-layer caveat. After the counter-examples list, add something like:
Note: Adapter-level preprocessing that runs before
{prompt}is constructed (e.g.resolve_mentions()stripping the bot's own trigger mention) is not subject to this rule. Rule 1 applies to transformations on the already-constructed{prompt}within the broker/dispatcher pipeline.This makes the boundary explicit and prevents false-positive compliance flags.
-
§4.4 Phase 1 test list — consider adding a
resolve_mentionsintegration test. Since the ADR explicitly prohibits mention expansion as a counter-example, a test verifying that<@user_id>mentions pass through the packing pipeline unchanged would anchor the prohibition in code. -
§6.6 threshold formula clarity (re-raised from @obrutjack's previous review, Suggested Change #2). The formula
p95_batch_size × context_tokens_per_event > 500 tokensmixes count and token units. Consider clarifying: "...where the product represents the estimated per-dispatch envelope overhead in tokens contributed by<sender_context>headers."
⚪ Nits
-
Self-reference in metadata. The "Supersedes" field says
[PR #598]— this PR supersedes itself, which is technically accurate (old content replaced by new) but reads oddly. Consider changing to "Supersedes: standalonebatched-turn-packing.md(original content of this PR)". -
§3.2
slack_ts_to_iso8601— still listed as if it exists. Worth marking as "(proposed helper)" per @obrutjack's previous nit.
Review Summary
🔴 Blockers
None.
💬 Questions
- §6.4 rule 1 — clarify adapter-layer vs broker-layer scope for
resolve_mentions(re-raised from previous review cycle)
🔧 Suggested Changes
- Add adapter-layer caveat to §6.4 rule 1
- Add
resolve_mentionspassthrough integration test to Phase 1 test list - Clarify threshold formula units in §6.6
⚪ Nits
- Self-referencing "Supersedes" field
- Mark
slack_ts_to_iso8601as proposed helper
Verdict
COMMENT — requesting contributor response on the §6.4 rule 1 scope clarification (Question #1, re-raised from previous review cycle). The ADR is exceptionally well-written and the design is sound. Once the adapter-layer boundary is clarified, this is ready for maintainer approval.
OpenAB PR ScreeningThis is auto-generated by the OpenAB project-screening flow for context collection and reviewer handoff.
Screening report## IntentPR #598 adds an ADR documenting how batched turn-boundary messages should be represented when crossing the broker -> ACP boundary. It addresses a concrete attribution problem: when multiple arrival events are flattened into one ACP turn, attachments and extra content blocks can lose their association with the original message/sender context. FeatThis is a docs/architecture decision PR. Behaviorally, it does not implement batching. It records the intended packing model:
Who It ServesPrimary beneficiaries:
Rewritten PromptReview and merge the ADR for turn-boundary message batching if it accurately captures the intended broker -> ACP packing contract. Validate that
Do not implement batching in this PR. Flag only ADR clarity, consistency with existing adapter/gateway architecture, and whether the decision gives enough guidance for the follow-up implementation. Merge PitchThis PR is worth advancing because it turns an unsettled batching design into a concrete architectural contract before code lands. That lowers implementation ambiguity and gives reviewers a single place to challenge the packing model. Risk is low because this is documentation-only. The main reviewer concern is whether the ADR over-specifies a design that may still change during implementation, especially around Best-Practice ComparisonOpenClaw principles that are relevant:
OpenClaw principles that are not directly in scope:
Hermes Agent principles that are relevant:
Overall, the ADR aligns with the shared best-practice theme of preserving execution context explicitly instead of relying on positional inference. Implementation OptionsOption 1: Conservative ADR-only merge Option 2: ADR plus implementation checklist Option 3: ADR plus minimal contract tests in follow-up branch Option 4: Ambitious batching implementation series Comparison Table
RecommendationAdvance with Option 2: merge the ADR after ensuring it includes a crisp implementation checklist and clearly states what is deferred. That gives Masami or Pahud a tighter next brief without expanding this PR beyond documentation. The likely follow-up should be split into a small contract-test PR first, then the actual broker/dispatch implementation once the packed turn shape is locked. |
chaodu-agent
left a comment
chaodu-agent
left a comment
There was a problem hiding this comment.
LGTM ✅ — Exceptionally thorough ADR that establishes a clear architectural contract for turn-boundary batching. Ready to merge.
Baseline Check
Main has 4 existing ADRs, per-message dispatch model, no batching mechanism. SenderContext has no timestamp field.
PR adds single new file: docs/adr/turn-boundary-batching.md (1051 lines). No code changes — purely architectural decision record. Documents mechanism (per-thread bounded mpsc::channel + consumer task), packing format (repeated <sender_context> template), config schema, phased rollout, alternatives considered, compliance rules, and reference implementation sketch.
PR #686 (implementation) already exists and depends on this ADR, confirming it is actively needed.
四問框架
- What problem? Multiple messages during in-flight turns become separate sequential ACP turns — wasting tokens, losing attachment attribution, non-deterministic ordering.
- How? Per-thread bounded
mpsc::channelwith consumer task. Three invariants: I1 (zero latency on first message), I2 (at most one in-flight turn per thread), I3 (broker structural fidelity — no merging, splitting, reordering, or semantic transformation). - Alternatives? Exhaustively documented in §5: pre-turn debouncing, single-slot overwrite (Hermes), mid-turn interrupt, topic detection, mutex-level coalescing,
<message index=N>wrapper, sidecar metadata, injected banner. All rejected with clear reasoning. - Best approach? Yes. Turns the constraint (no mid-turn interrupt for external ACP CLIs) into a feature. Phased rollout (opt-in → validation → default flip) is conservative and appropriate.
Traffic Light
🟢 INFO — I3 (Broker Structural Fidelity) is load-bearing and well-articulated with 5 prohibited transformations and 9 compliance rules
🟢 INFO — Single uniform code path for N=1 and N≥2 — no special-case branches
🟢 INFO — Prior art analysis with Hermes and OpenClaw is honest and detailed (§5.3)
🟢 INFO — Observability is Phase 1 must-have, not a follow-up (§6.6)
🟢 INFO — Error handling design (§2.5) with generation-based race-safe eviction is thorough
🟡 NIT — PR description filename mismatch: says batched-turn-packing.md but actual file is turn-boundary-batching.md
🟡 NIT — Self-referential "Supersedes" header reads oddly — consider rewording
🟡 NIT — §3.2 timestamp source for Gateway adapter could note it's receive-time, not platform-time
This comment has been minimized.
This comment has been minimized.
|
Heads-up from #686 review (chaodu-agent 🔴 #1): when this ADR's implementation ( On main,
ADR §3.5's uniform "header + extra_blocks in arrival order" template puts all extra_blocks after the header. This is intentional, but it changes runtime behavior even in Suggested addition to the ADR: a short "Migration / behavior change" note documenting this ordering shift so the implementation PR can link here instead of restating the rationale. |
This comment has been minimized.
This comment has been minimized.
PR Review: #598 — docs: add ADR for turn-boundary message batchingAPPROVE ✅ — Comprehensive, well-structured ADR that establishes a clear architectural contract for turn-boundary batching. Ready for maintainer merge. Four-Question Framework1. What problem does it solve?Within one thread, messages arriving during an in-flight ACP turn become independent sequential turns, wasting intermediate output (token cost), losing attachment attribution, and producing non-deterministic ordering. The ADR documents the design decision for a per-thread bounded 2. How does it solve it?
3. What was considered?Six mechanism alternatives (§5.1): pre-turn debouncing, single-slot overwrite (Hermes pattern), mid-turn interrupt, topic detection, cross-thread keying, mutex coalescing — all rejected with clear rationale. Four packing alternatives (§5.2): RFC MVP wrapper-and-flatten, wrapper with inline extras, asymmetric ordering special case, banner injection — all rejected. Prior art comparison (§5.3) against Hermes Agent and OpenClaw with source-level analysis. 4. Is this the best approach?Yes. The design turns an architectural constraint (no mid-turn interrupt for external ACP CLIs) into a feature. The three invariants are well-defined and the compliance rules (§6.4) provide a durable test surface for future implementation PRs. The phased rollout (Phase 1 default=per-message, Phase 3 default flip) is conservative and safe. Detailed Findings🟢 INFO — Strengths
🟡 NIT — Non-blocking suggestions
Previous review findings status
VerdictThe ADR is exceptionally thorough at 1051 lines. The design is sound, the compliance rules are durable, and the phased rollout is safe. The The obrutjack |
chaodu-agent
left a comment
chaodu-agent
left a comment
There was a problem hiding this comment.
APPROVE ✅ — Comprehensive ADR with sound design, durable compliance rules, and safe phased rollout. No blocking issues.
github-actions
Bot
added
the
closing-soon
…e 1 implementation Updates the ADR to match decisions made during PR openabdev#686 review: - 3-valued MessageProcessingMode (per-message / per-thread / per-lane) replacing the earlier 2-valued (per-message / batched) design. §4.1 documents per-mode (cap, dispatcher key, idle timeout) tuple; §4.4 Phase 1 bullets reflect the unified Dispatcher::submit path; legacy "batched" alias is rejected at config parse. - Standalone <sender_context> Text block (commit 072010c). §3.1 / §3.3 / §3.4 / §3.5 / §3.6 + §6.4 rule 4 now describe the split-block layout: delimiter + transcripts + prompt + images. Transcripts move from before the envelope to inside the arrival event (between delimiter and prompt); images stay after prompt as in the pre-batching adapter. Empty prompt is omitted from the block stream. - New §6.10 — per-mode consumer idle timeout (PER_MESSAGE = 10s, DEFAULT = 300s) with Little's-Law rationale and sweep_stale eviction. - New §6.11 — SendError manual staging smoke matrix (the entry deferred out of CI in PR openabdev#686's first status update). - §6.7 batch reactions now explicitly sequential (not join_all parallel) so reaction-list ordering across a batch matches message-ID order. - Frontmatter: drop the self-referential "Supersedes: PR openabdev#598" line; add "Implementation PR: openabdev#686" so the ADR points at the wiring it documents.
github-actions
Bot
removed
the
closing-soon
PR Review: #598Based on commit: Summary
Core Assessment
Findings🔴 Critical: PR description filename does not match actual fileWhere: PR description body vs diff What's wrong: The PR description states:
But the actual file in the diff is Why it matters: Reviewers and future readers searching for the file by the name in the PR description will not find it. This also means the test plan item "ADR renders in Fix: Update the PR description to reference the correct filename 🟡 Minor: Self-referential entry in References sectionWhere: What's wrong: The References section includes:
This PR is #598. The entry says it's "superseded" and "folded into §3 of this document" — but this document is PR #598. This creates a confusing self-reference. Why it matters: Future readers will click the link, land on this same PR, and be confused about what was "superseded" and what was "folded." It reads as if there was a prior PR #598 that was replaced, but there wasn't — this is the only #598. Fix: Either remove this entry entirely, or rewrite it to clarify the history. If the intent is to note that an earlier draft of this ADR was restructured, say that in the changelog (which already does — v0.1 → v0.2 → v0.3). If there was a separate earlier PR that was closed in favor of this one, reference that PR number instead. Review Summary (Traffic Light)🟢 INFO
🟡 NIT
🔴 SUGGESTED CHANGES
VerdictCOMMENT — The 🔴 is a PR description fix (not a code/doc content issue), so it's trivial to address. The ADR content itself is comprehensive and well-structured. Once the description is corrected, this is ready for maintainer review. |
…enabdev#686 head Five rounds of fact-check + proofread against PR openabdev#686 (feature/turn-boundary-batching-v2 @ e119abf) caught two threads of drift: - Design contract: §2.5 SendError handler now matches commit afd6fff — proactive consumer.is_finished() check at submit head + transparent retry once on SendError; ❌ +⚠️ + Err(ConsumerDead) only if the retry also fails. Motivated by the first-message-after-idle race; one-attempt bound preserves the no-spin-loop property. §6.11 staging smoke matrix split into Path A (PANIC_ONCE happy path, no user-visible signal) and Path B (PANIC_ALWAYS failing-retry surfaces ❌+⚠️ ). §4.4 Phase 1 plan + test list updated to the new contract. - Anchor audit vs declared base v0.8.2-beta.1 (52052b8): pre-existing drift fixed in adapter.rs references that had been wrong since the SHA pin was set in v0.2 — :131-152→:156-172, :138-143→:158-162 (7 sites), :148-152→:165-169, :154-161→:173-180, :181→:254 (was pointing at the wrong call), :240→:260. acp/connection.rs / acp/pool.rs / discord.rs / slack.rs anchors verified clean against 52052b8. - §2.6 rewritten: other_bot_present is a bool snapshot carried on BufferedMessage and read from batch.last() at dispatch time — not the Arc<AtomicBool> mirror of an earlier draft. §2.3 struct + submit signature corrected to match. - Anchor-pinning preamble (line 9) expanded to pin both SHAs explicitly: released-code anchors → 52052b8; conceptual descriptions of new modules → cross-checked against e119abf. - Appendix A replaced with a signatures-only skeleton pointing at src/dispatch.rs — drops the ~200-line body sketch that had drifted from the implementation; rationale moved into a short shape-choices list. - Path anchors swept: pool.rs → acp/pool.rs, connection.rs → acp/connection.rs (modules live under src/acp/ in v0.8.2-beta.1). - §6.6 metric table cell tokens_per_event (was context_tokens_per_event, inconsistent with the code block immediately below). Co-Authored-By: Claude Opus 4.7 <[email protected]>
Update — fact-check pass against PR #686 headPushed 1. Design contract change: §2.5 SendError handlerCommit
2. Pre-existing anchor drift — fixedThe ADR's preamble pins released-code
Other notable changes
Diff stat
Ready for re-review. |
🟢 PR #598 — docs: add ADR for turn-boundary message batchingVerdict: Approve What problem does it solve?Documents the design rationale for turn-boundary message batching — the mechanism that collects messages arriving during an in-flight ACP turn and dispatches them as a single batch at turn completion. This ADR is the design companion to the implementation in PR #686. How does it solve it?A comprehensive 1068-line ADR (
What was considered?The ADR itself documents alternatives extensively in §5 (debouncing rejected for latency floor, message-index wrapper rejected for schema complexity, etc.). The three-way comparison table (§3.4) clearly shows why repeating the existing Is this the best approach?Yes. This is an exemplary ADR:
Detailed notes🟢 INFO — Well done
🟡 NIT — Non-blocking suggestions
🔴 SUGGESTED CHANGES — NoneNo blocking issues found. |
…ts, fix ADR path - main.rs: collapse 3x repeated (cap, grouping, idle) match blocks into dispatch::dispatch_params(mode, max_buffered). - dispatch.rs: replace magic 4 / 512 in estimate_tokens with named CHARS_PER_TOKEN_ESTIMATE / TOKENS_PER_IMAGE_ESTIMATE constants. - dispatch.rs: fix top-level ADR reference to point at the actual docs/adr/turn-boundary-batching.md path landing in openabdev#598. Addresses chaodu-agent NITs openabdev#1, openabdev#2, openabdev#5 from PR openabdev#686. Co-Authored-By: Claude Opus 4.7 <[email protected]>
|
@masami-agent re your
@chaodu-agent re your
|
|
LGTM ✅ — Thorough ADR documenting the turn-boundary batching design. Well-synchronized with the implementation in PR #686. Ready to merge (should land before #686). 📋 Review — Four Questions1. What problem does this solve?Documents the design decision for turn-boundary message batching — how N messages arriving during an in-flight ACP turn are buffered and dispatched as a single batch. Extracted from RFC #580 into a standalone ADR. 2. How does it solve it?1068-line ADR covering:
3. What alternatives were considered?ADR §5 documents rejected alternatives:
4. Is this the best approach?Yes. The ADR is well-structured, fact-checked against the implementation (PR #686), and properly anchored to specific commits. 🟢 INFO — Well done:
🟢 No issues found. This is a well-maintained living ADR that accurately reflects the implementation. 🔍 Baseline Check (Step 0)
|
chaodu-agent
left a comment
chaodu-agent
left a comment
There was a problem hiding this comment.
docs/adr/turn-boundary-batching.md line 807: Rule 8 contradicts the SendError decision in §2.5. §2.5 says a first SendError should evict, spawn a fresh consumer, and re-send once with no user-visible signal when the retry succeeds; only a failed retry should emit ❌ +
* feat(dispatch): turn-boundary batching dispatcher v2 per ADR v0.3 * refactor(dispatch): cleanup naming, parallelize queued reactions, use configured emoji on SendError - Rename ThreadHandle._consumer → consumer (we actually .abort() it on cancel) - Replace ThreadHandle::drain_pending(&mut self) with pending_count(&self) — read-only signature, name no longer implies side effects - Parallelize 👀 reactions in dispatch_batch via futures::join_all instead of serial loop — first-token latency no longer scales with batch size - SendError ❌ reaction now uses router.reactions_config() instead of ReactionsConfig::default() — respects user-configured emoji - shutdown() switches to iter() (no longer needs &mut after the rename above) - Tighten doc comments - Cargo.lock: sync to openab 0.8.2 (Cargo.toml already at 0.8.2) * feat(discord): add /cancel-all slash command Adds the standalone /cancel-all path from ADR §4.4 turn-boundary batching. Unlike /reset, /cancel-all is non-destructive to the session. - /cancel-all: dispatcher.cancel_buffered() + pool.cancel_session() → drops buffered messages + aborts in-flight ACP turn, keeps session - /reset: unchanged (still drops buffered + cancels in-flight + tears down session); doc comment updated to reflect that /reset is a superset of /cancel-all rather than "/reset includes /cancel-all" Discord-only — Slack adapter explicitly drops slash_commands envelopes (no thread routing on channel-level delivery), Gateway has no user-facing slash command surface. Response messages cover all four (cancel_session result × dropped count) cases. * refactor: unify PerMessage and Batched modes through Dispatcher Both modes now serialize through the per-thread Dispatcher consumer task. PerMessage = max_buffered_messages=1 (each message dispatches alone, FIFO). Batched = configured cap (greedy drain up to max_batch_tokens). Removes the bifurcated match in Slack/Discord/Gateway hot paths, eliminates the Option<Arc<Dispatcher>> indirection, and addresses chaodu-agent PR #686 review concern about PerMessage FIFO regression after the KeyedAsyncQueue removal. * chore(dispatch): address PR #686 NITs - Extract duplicated days_to_ymd / ISO 8601 conversion from slack.rs + gateway.rs into new src/timestamp.rs (with unit tests). - Add sender_name to BufferedMessage per ADR §2.3 — denormalised from sender_json so dispatch_batch tracing doesn't pay a JSON parse. - impl std::error::Error for DispatchError so it composes with anyhow. * fix(dispatch): idle eviction, config validation, avoid clone, timestamp precision - Add 5-min idle timeout to consumer_loop to prevent per-thread handle/task leak (unbounded growth from one-shot thread keys like Slack non-thread msgs) - Validate max_buffered_messages > 0 at config load time (prevents panic from tokio::sync::mpsc::channel(0)) - Use into_iter() in dispatch_batch to avoid deep-copying extra_blocks (may contain base64 image data) - Add TODO comment for gateway multibot detection - Use real milliseconds in now_iso8601() via dur.subsec_millis() Co-authored-by: 超渡法師 <[email protected]> * fix(dispatch): proactive stale-entry cleanup + transparent retry on idle exit - submit() now checks consumer.is_finished() before using an existing handle, removing stale entries proactively (fixes map leak for one-shot thread keys that never get a second submit) - On SendError, transparently evict + rebuild + retry once instead of surfacing an error to the user (fixes first-message-after-idle being treated as ConsumerDead) - Only report ConsumerDead if the retry also fails (truly unexpected) * fix(dispatch): periodic sweep of stale per-thread entries - Add Dispatcher::sweep_stale() that retains only entries whose consumer task is still running (map.retain + is_finished check) - Wire into main.rs cleanup task (60s interval, alongside pool.cleanup_idle) - Prevents unbounded map growth from one-shot thread keys (e.g. Slack non-thread messages) that never receive a second submit() - dispatchers Vec wrapped in Arc<Mutex<>> so cleanup task can access it * feat(dispatch): add per-lane batching mode (default for "batched" alias) Extends MessageProcessingMode from {PerMessage, Batched} to three values: - PerMessage: each message → one ACP turn (unchanged default behaviour) - PerThread: thread-wide buffer, all senders share one batch (old "Batched") - PerLane: per (thread, sender) buffer, each sender gets its own ACP turn The legacy alias "batched" now resolves to PerLane — the recommended default for batching, since per-lane eliminates the silent-drop risk where a single mixed-sender ACP turn produces one reply that may forget to address some senders. Existing configs continue to load without change but now run under per-lane semantics. Implementation: - Adds BatchGrouping enum to dispatch.rs and `Dispatcher::key()` helper that builds the per-thread map key from (platform, thread_id, sender_id). PerThread mode ignores sender_id; PerLane includes it. - main.rs translates MessageProcessingMode to (cap, BatchGrouping) when constructing each platform's Dispatcher. - Discord/Slack/Gateway adapters use `dispatcher.key(...)` instead of hand-rolled format!() at submit and slash-command sites. - Session pool keys remain per-thread (unchanged) — the ACP session is shared across lanes by design; turns serialise through the shared session. - /cancel-all and /reset use the invoker's lane key (B1: cancel only own lane) but still cancel/reset the shared session (B4-a: keep escape hatch from a runaway in-flight turn). Tests: - dispatch::tests::key_per_thread_ignores_sender / key_per_lane_includes_sender - config::tests::message_processing_mode_{parses_per_message,parses_per_thread, parses_per_lane,batched_alias_is_per_lane,default_is_per_message, unknown_value_errors} - 224 tests passing. Co-Authored-By: Claude Opus 4.7 <[email protected]> * fix(dispatch): /reset and /cancel-all clear all lanes in thread Replaces the per-key Dispatcher::cancel_buffered with cancel_buffered_thread, which prefix-matches every per-thread handle for a (platform, thread_id) pair and aborts each consumer. Both PerThread keys (`platform:thread`) and PerLane keys (`platform:thread:sender`) are dropped, with care taken to avoid the substring trap (T1 must not match T10). Behaviour: - /cancel: unchanged — stop in-flight ACP turn only, queue continues. - /cancel-all: stop in-flight + drop every lane's buffer in the thread (was: invoker's lane only). The nuclear escape hatch — keeps ACP context, clears queued work so a human can intervene. - /reset: drop every lane's buffer + tear down the ACP session (was: invoker's lane only). Next message in the thread starts a fresh session. Gateway: - run_gateway_adapter now also receives the AdapterRouter, so the upstream /reset and /cancel slash-command interception (added on main while this branch was in review) compiles after rebase. - Gateway /reset gets the same all-lanes drop as Discord; /cancel keeps the in-flight-only semantics from upstream. - /cancel-all is intentionally not added to the gateway interception path. Tests: 227 passing (+3 new dispatcher tests covering PerThread drop, PerLane all-lanes drop, and the T1-vs-T10 prefix-collision guard). Co-Authored-By: Claude Opus 4.7 <[email protected]> * feat(config)!: drop "batched" alias, only per-message/per-thread/per-lane accepted The legacy `"batched"` value (which resolved to PerLane on this branch) is removed. Configs using `message_processing_mode = "batched"` will now fail to parse with an `unknown variant "batched"` error pointing at the three accepted values, forcing an explicit migration to per-thread or per-lane. The two batching modes have meaningfully different semantics (shared vs isolated buffer per sender), so a silent default is the wrong call — users should pick deliberately. Co-Authored-By: Claude Opus 4.7 <[email protected]> * fix(dispatch): restore shared thread sessions and abort consumers on shutdown Co-authored-by: Brett Chien <[email protected]> * feat(chart): expose message_processing_mode and batching params Adds messageProcessingMode / maxBufferedMessages / maxBatchTokens to the Discord, Slack, and Gateway sections of the chart. Without these the turn-boundary batching modes shipped in PR #686 are unreachable from a helm-deployed instance — the Rust binary just falls back to per-message. - configmap.yaml: render the three keys for each platform when set, with enum validation matching the Rust deserializer ("must be one of: per-message, per-thread, per-lane"). - values.yaml: commented examples for each platform. - tests/message-processing-mode_test.yaml: 12 helm-unittest cases covering render, enum rejection, omit-when-unset, and numeric param render across all three platforms. Co-Authored-By: Claude Opus 4.7 <[email protected]> * refactor: rename enum variants to drop redundant Per prefix Aligns MessageProcessingMode and BatchGrouping with the rest of the codebase (TableMode, AllowBots, ToolDisplay, TurnSeverity, etc.) where variants don't repeat the enum-name-derived prefix. Also fixes the CI clippy::enum_variant_names failure on PR #686. Wire format unchanged — manual Deserialize still matches per-message / per-thread / per-lane strings; helm chart and TOML configs need no edits. - MessageProcessingMode { PerMessage, PerThread, PerLane } -> { Message, Thread, Lane } - BatchGrouping { PerThread, PerLane } -> { Thread, Lane } * feat(config): validate max_batch_tokens > 0 Setting max_batch_tokens=0 doesn't crash but forces every batch to size 1 via the consumer loop's token-cap check — functionally per-message mode through a confusing path. Reject it at config parse time, alongside the existing max_buffered_messages > 0 check. Co-Authored-By: Claude Opus 4.7 <[email protected]> * test(dispatch): cover sweep_stale and shutdown Add an alive_consumer_handle helper (parks on pending::<()>) and four unit tests: - sweep_stale removes finished consumers, leaves running ones alone - shutdown clears the per-thread map and aborts running consumers (verified via abort_handle().is_finished() after a runtime tick) These paths are simple but safety-critical (SIGTERM cleanup + idle-task GC); the existing dummy_handle / make_dispatcher scaffolding already covers the test surface, so no new mocks needed. Co-Authored-By: Claude Opus 4.7 <[email protected]> * test(dispatch): cover consumer_loop via DispatchTarget trait seam Closes the NIT 2 gap from PR #686 review. The consumer_loop orchestration (greedy drain / token cap overflow / idle timeout / SendError eviction) was previously only verified by manual staging smoke. The trait seam also unblocks the §2.5 SendError end-to-end test. Refactor: - DispatchTarget trait (reactions_config / ensure_session / stream_prompt_blocks) extracted from AdapterRouter's surface. AdapterRouter implements it by delegation. - Dispatcher now holds Arc<dyn DispatchTarget>. Production callsites unchanged — Arc<AdapterRouter> auto-coerces via CoerceUnsized. - Add Dispatcher::with_idle_timeout (test knob); Dispatcher::new keeps the DEFAULT_CONSUMER_IDLE_TIMEOUT (5 min) production default. Tests: - MockDispatchTarget records dispatches; MockChatAdapter is a no-op stub. - consumer_dispatches_single_message_as_one_batch (happy path) - consumer_greedy_drain_combines_queued_messages_into_one_batch (3 pre-loaded msgs → 1 dispatch with 3 ContentBlocks) - consumer_token_cap_splits_batch_preserving_fifo (2x 80-token msgs + cap=100 → 2 FIFO dispatches) - consumer_exits_after_idle_timeout_with_no_messages (50ms timeout) - submit_evicts_dead_handle_and_retries_with_fresh_consumer (manufactured dead handle: rx dropped, consumer parked → SendError → eviction + retry on fresh consumer) Co-Authored-By: Claude Opus 4.7 <[email protected]> * fix(adapter): make SenderContext.timestamp truly additive Wraps the field in Option<String> with skip_serializing_if so consumers that pre-date the addition see no new key in the serialized JSON. All four producers (slack, discord, gateway, cron) wrap their existing values in Some(...). Schema string stays openab.sender.v1. * docs(dispatch): note re-acquire-after-await safety in submit Calls out why re-acquiring per_thread after tx.send().await cannot deadlock — the first lock guard is dropped before the await point. * fix(adapter): use sender_context as standalone delimiter, split prompt into own block pack_arrival_event now emits per arrival: [Text "<sender_context>{json}</sender_context>"] (delimiter) [Text transcript blocks from extra_blocks] [Text prompt] (omitted if empty) [non-Text blocks (e.g. Image)] The sender_context block stands alone as a structural delimiter so agents can locate arrival boundaries by scanning for `<sender_context>` openers in batched dispatch. Within each arrival, transcript text precedes the typed prompt to match pre-batching adapter UX (voice content first), and images trail the prompt as before. Tests updated to reflect the new per-arrival block count (2 minimum: delimiter + prompt; +1 per transcript; +N for image attachments). * fix(gateway): import AdapterRouter so handle_config_command compiles handle_config_command's signature uses &AdapterRouter but only crate::adapter::{ChannelRef, ChatAdapter, MessageRef, SenderContext} were imported, so cargo check failed with E0425. Add AdapterRouter to the use list (the other reference at line 482 already uses the fully qualified path). * fix(timestamp): parse Slack ts as f64 to preserve decimal semantics Previously slack_ts_to_iso8601 split on '.' and parsed the fractional substring as an integer, treating ".12" as 12 ms instead of 120 ms. Parsing the entire string as f64 carries decimal semantics correctly without any string-padding logic. Co-Authored-By: Claude Opus 4.7 <[email protected]> * refactor(discord): drop approximate count from /cancel-all message The buffered-message count is approximate (sweep races with new arrivals) so surfacing an exact number to users was misleading. Show a binary "cleared / nothing" signal instead. The pending_count() API stays for logs and metrics. Co-Authored-By: Claude Opus 4.7 <[email protected]> * docs(dispatch): annotate per_thread mutex lock sites with SAFETY comments Make the no-.await-while-locked invariant explicit at each lock acquisition site so future edits can't silently introduce an .await without tripping the comment. The struct-level note at line 183 stays as the higher-level explanation. Co-Authored-By: Claude Opus 4.7 <[email protected]> * refactor(dispatch): apply queued reactions sequentially Replace futures_util::future::join_all with a sequential await loop. Batches are typically small (low single digits) so the serialization cost is sub-second and not user-visible, and the dispatch path no longer pulls in join_all just for one call. Co-Authored-By: Claude Opus 4.7 <[email protected]> * refactor(dispatch): per-mode consumer idle timeout (10s for per-message) Per-message mode (cap=1) doesn't benefit from holding consumers across message gaps — there is no batch window to preserve — so a 5-minute idle timeout left consumer tasks lingering long after they were useful. Add PER_MESSAGE_CONSUMER_IDLE_TIMEOUT (10s), wire it through main.rs based on each adapter's message_processing_mode, and drop the unused Dispatcher::new wrapper. By Little's Law (steady-state idle count = arrival rate × idle window), this cuts per-message-mode idle dispatcher footprint by 30x for the same arrival rate while keeping batched modes' 5-minute window so between-trigger lanes aren't torn down on every message. Co-Authored-By: Claude Opus 4.7 <[email protected]> * refactor(dispatch): extract dispatch_params, name token-estimate consts, fix ADR path - main.rs: collapse 3x repeated (cap, grouping, idle) match blocks into dispatch::dispatch_params(mode, max_buffered). - dispatch.rs: replace magic 4 / 512 in estimate_tokens with named CHARS_PER_TOKEN_ESTIMATE / TOKENS_PER_IMAGE_ESTIMATE constants. - dispatch.rs: fix top-level ADR reference to point at the actual docs/adr/turn-boundary-batching.md path landing in #598. Addresses chaodu-agent NITs #1, #2, #5 from PR #686. Co-Authored-By: Claude Opus 4.7 <[email protected]> * docs: clarify schema evolution comment + dispatchers triple-Arc rationale - adapter.rs: note that future breaking changes should bump to v1.1+ - main.rs: explain why Arc<Mutex<Vec<Arc<Dispatcher>>>> is necessary (shared with cleanup task + shutdown; pushes at startup only) Addresses maintainer NITs from PR #686 review. Co-Authored-By: 超渡法師 <[email protected]> * docs: add message dispatch modes guide (per-message vs per-thread vs per-lane) Decision guide for operators choosing between the three modes, with config examples and trade-off explanations. Co-Authored-By: 超渡法師 <[email protected]> * docs(dispatch): add ASCII diagrams for all three modes + consumer loop Visual explanation of per-message vs per-thread vs per-lane behavior, plus the internal consumer_loop batching flow. Co-Authored-By: 超渡法師 <[email protected]> * docs: clarify per-message is the default behavior * docs(dispatch): add explicit pros/cons and comparison table for each mode --------- Co-authored-by: Brett Chien <[email protected]> Co-authored-by: 超渡法師 <[email protected]> Co-authored-by: Claude Opus 4.7 <[email protected]> Co-authored-by: shaun-agent <[email protected]> Co-authored-by: brettchien <[email protected]> Co-authored-by: chaodu-agent <[email protected]>
7 participants
Discord Discussion URL: https://discord.com/channels/1491295327620169908/1497977225314832536
Summary
Adds a standalone ADR (
docs/adr/turn-boundary-batching.md) recording how a batched ACP turn is packed intoVec<ContentBlock>across the broker → ACP boundary, extracted from RFC #580 (Turn-boundary message batching).The ADR converges on repeating the existing per-arrival-event
<sender_context>template N times rather than introducing the<message index=N>wrapper schema the RFC MVP proposed. A single additivetimestampfield onSenderContext(schema staysopenab.sender.v1) gives arrival-event distinguishability and subsumes T2.j'sarrived_at_relativeproposal.Closes the attribution gap surfaced independently by:
extra_blockstail loses the attachment ↔ message linkWhat this PR does and does not include
pack_arrival_event/dispatch.rsimplementation referenced inside the ADR — that lands separately.Test plan
docs/adr/./multi-platform-adapters.mdand./custom-gateway.mdresolveTracking: #580