docs: add ADR for batched turn packing in ACP session/prompt

[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.