docs: add ADR for turn-boundary message batching

[brettchien]

Discord Discussion URL: https://discord.com/channels/1491295327620169908/1497977225314832536

Summary

Adds a standalone ADR (docs/adr/batched-turn-packing.md) recording how a batched ACP turn is packed into Vec<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 additive timestamp field on SenderContext (schema stays openab.sender.v1) gives arrival-event distinguishability and subsumes T2.j's arrived_at_relative proposal.

Closes the attribution gap surfaced independently by:

• Community Triage review (T1.4) — flattened extra_blocks tail loses the attachment ↔ message link
• JARVIS + FRIDAY review (B1) — same gap, framed independently

What this PR does and does not include

• Includes: the ADR document only.
• Does not include: the pack_arrival_event / dispatch.rs implementation referenced inside the ADR — that lands separately.

Test plan

• ADR renders in docs/adr/
• Sibling links to ./multi-platform-adapters.md and ./custom-gateway.md resolve
• Reference links to GitHub issue comments resolve

Tracking: #580

[thepagent]

request input from @masami-agent and @shaun-agent

[masami-agent]

PR Review: #598

Summary

• Problem: RFC #580 MVP packing flattens extra_blocks to a tail, destroying attachment ↔ message attribution (flagged independently by T1.4 and B1).
• Approach: Reuse existing <sender_context> template repeated N times with one additive timestamp field — no new schema, no wrapper tags.
• Risk level: Low (docs-only ADR, no code change in this PR)

Core Assessment

1. Problem clearly stated: ✅ — grounded in two independent reviews (T1.4, B1) that converged on the same gap
2. Approach appropriate: ✅ — reuses existing <sender_context> rather than inventing parallel schema; minimal surface area
3. Alternatives considered: ✅ — six alternatives (§6.1–6.5) each with clear rejection rationale
4. Best approach for now: ✅ — single uniform code path, no ACP protocol change, additive schema evolution

Findings

Verified against current codebase:

• SenderContext struct in adapter.rs currently has: schema, sender_id, sender_name, display_name, channel, channel_id, thread_id (optional), is_bot — no timestamp field, confirming the ADR's claim that the addition is purely additive ✅
• handle_message in adapter.rs does prepend Text extra_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_sender format 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_id omission: The SenderContext struct has #[serde(skip_serializing_if = "Option::is_none")] on thread_id, so it's absent from serialized JSON when None. 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 with skip_serializing_if behavior, such as thread_id, are omitted when None) 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 for serde with #[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-152 are ~20 lines off from the current code (the logic moved down due to added comments and the thread_id field). 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 actual Vec<ContentBlock> array these would be separate ContentBlock entries 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.

[masami-agent]

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 1

This 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-have

The ADR defines three metrics for monitoring token cost growth (§7 Negative consequences):

• context_tokens_per_event
• p95_batch_size
• packed_block_count

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 1

The ADR acknowledges a behavior change for voice-only messages (STT transcripts move from before <sender_context> to after) and proposes a rollback path: "hotfix PR if cross-agent smoke fails." However, the smoke test scope is not defined — which agents to test, what the pass criteria are, or who runs it.

Suggestion: Before the Phase 1 implementation PR is opened, define a minimal smoke test matrix:

• Agents: at minimum Claude Code and Cursor (the two most common ACP agents in production)
• Test case: voice-only message → verify the transcript content appears in the agent's response
• Pass criteria: agent references or acknowledges the transcript content (not just emoji reaction)

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]

PR Review: #598

Summary

• Problem: RFC #580 MVP packing flattens extra_blocks to a tail, destroying attachment ↔ message attribution (T1.4 / B1).
• Approach: Reuse existing <sender_context> template with additive timestamp field; attachments attributed by array adjacency.
• Risk level: Low (docs-only PR, no runtime changes)

Core Assessment

1. Problem clearly stated: ✅
2. Approach appropriate: ✅
3. Alternatives considered: ✅ (§6 covers 5 alternatives with clear rejection rationale)
4. Best approach for now: ✅

Source Code Verification

Verified against current main:

• adapter.rs SenderContext struct confirms no timestamp field — additive change is accurate
• adapter.rs:handle_message confirms transcript Text blocks prepended, image blocks appended — §3.5 / Scenario D behavior change description is accurate
• discord.rs confirms voice transcript uses extra_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

1. §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

1. §8 Compliance rule 1 lists "broker expanding Discord <@123> mentions to @username strings" as a prohibited transformation, but discord.rs's resolve_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.
2. §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

1. §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.

[brettchien]

Updated this PR to expand the ADR's scope.

The original batched-turn-packing.md covered only the packing format. The new turn-boundary-batching.md consolidates the full scope of issue #580 — mechanism (per-thread mpsc + consumer task at turn boundary), packing format, configuration & rollout phases, alternatives considered, prior-art comparison (Hermes / OpenClaw), and compliance rules.

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]

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

1. Problem clearly stated: ✅ — three concrete workload patterns (§1.1), grounded in current code paths
2. Approach appropriate: ✅ — three invariants (I1/I2/I3) are well-defined and load-bearing for the rest of the document
3. Alternatives considered: ✅ — six mechanism alternatives (§5.1) + four packing alternatives (§5.2) + prior art comparison (§5.3), each with clear rejection rationale
4. 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: u64 counter with double-observer analysis is thorough. The explicit enumeration of what happens when two concurrent submit calls observe SendError on 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)

1. §6.4 rule 1 — resolve_mentions scope clarification needed.

   Rule 1 lists this as a prohibited counter-example:

   "broker expanding Discord <@123> mentions to @username strings"

   I verified the current resolve_mentions() in discord.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 flag resolve_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.

🔧 Suggested Changes

1. §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.

2. §4.4 Phase 1 test list — consider adding a resolve_mentions integration 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.

3. §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 tokens mixes count and token units. Consider clarifying: "...where the product represents the estimated per-dispatch envelope overhead in tokens contributed by <sender_context> headers."

⚪ Nits

1. 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: standalone batched-turn-packing.md (original content of this PR)".

2. §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

1. §6.4 rule 1 — clarify adapter-layer vs broker-layer scope for resolve_mentions (re-raised from previous review cycle)

🔧 Suggested Changes

1. Add adapter-layer caveat to §6.4 rule 1
2. Add resolve_mentions passthrough integration test to Phase 1 test list
3. Clarify threshold formula units in §6.6

⚪ Nits

1. Self-referencing "Supersedes" field
2. Mark slack_ts_to_iso8601 as 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.