Skip to content

perf(acp): coalesce streamed assistant text snapshots - #19502

Closed
Exotic209093 wants to merge 2 commits into
pingdotgg:mainfrom
Exotic209093:fix/acp-coalesced-text-snapshots
Closed

Exotic209093 wants to merge 2 commits into
pingdotgg:mainfrom
Exotic209093:fix/acp-coalesced-text-snapshots

Conversation

@Exotic209093

@Exotic209093 Exotic209093 commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

ACP agents emitted complete accumulated assistant and reasoning text on every token chunk. A burst of 200 assistant chunks produced 201 full text snapshots before considering the corresponding message and node events.

Route ACP root and child text through the existing scoped coalescer, shared through provider-core so the adapter package can use it. Token chunks flush at fifty-millisecond intervals; authoritative snapshots retain immediate projection. Segment changes, child completion, and carryover completion synchronously flush pending text before terminal events. Session teardown drains admitted native callbacks before flushing and closing the runtime. Event IDs, event formats, and complete final text are preserved.

Validation:

  • 11 initial focused ACP tests passed, covering bounded root/reasoning/child streaming, subagent paths, fidelity, and session-scope teardown. After review fixes, four focused ACP regressions passed, including resumed child message IDs.
  • 7 existing/shared coalescer tests passed, including authoritative replacement and Codex append/completion behavior.
  • The strengthened 200-chunk regression fails on the original adapter with 201 root snapshots; the fix emits at most ten while preserving every chunk of final root, reasoning, and child text. Child text precedes its completion event.
  • The scope-close regression proves child text is still buffered before the fifty-millisecond interval, closes the independent session scope, and receives the complete root/reasoning/child text.
  • The resumed-child regression observes the first intermediate projection with a typed receipt: A1 -> B1 -> A2 must emit A1A2 before terminal flush. It fails against published commit 151fa54112 with suffix-only A2 and passes with the fix; final A1A2A3 remains intact.
  • Scoped provider-acp, provider-core, and server typechecks, targeted lint and formatting, and git diff --check passed. Lint reports existing warnings in the server adapters/tests.

Closes #19461. This covers ACP agents; #15767 covers other providers and modifies the same coalescer, so its helper changes will need to follow the shared provider-core location when rebased.

Latest CI and CodeRabbit both passed on 19b9d1c50d. The child-message review finding is fixed and its thread is resolved.

Implemented with GPT-6.1-Sol through the Codex harness in T3 Code.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Oct 14, 2026
Comment thread packages/provider-acp/src/server/adapter.ts
@macroscopeapp

macroscopeapp Bot commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This PR changes production ACP streaming behavior by buffering and coalescing root and subagent text across multiple lifecycle paths, adding substantial state and teardown logic. An unresolved High-severity finding also identifies a scenario where resumed child-message text can be projected incompletely.

Not approved because:

  • 1 blocking correctness issue found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 1e20818c-56f6-428a-ae01-6df6abf2530c
📥 Commits

Reviewing files that changed from the base of the PR and between 7ac952c and 19b9d1c.

📒 Files selected for processing: 7
  • apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.test.ts
  • apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.ts
  • apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.test.ts
  • apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts
  • packages/provider-acp/src/server/adapter.ts
  • packages/provider-core/package.json
  • packages/provider-core/src/server/ProviderTextDeltaCoalescer.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

📝 Walkthrough

The provider-core text coalescer now supports replacing buffered text and flushing individual items. The ACP adapter uses it for root assistant, reasoning, and subagent text, with flushes at segment and session boundaries.

Changes: ACP text coalescing

Layer / File(s) Summary
Coalescer API and package integration
packages/provider-core/src/server/ProviderTextDeltaCoalescer.ts, packages/provider-core/package.json, apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.ts, apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts, apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.test.ts
The coalescer adds setText and flushItem, and the package exports it. Claude and Codex adapter imports use the package path. A test checks that setText replaces buffered text.
ACP text buffering and projection
packages/provider-acp/src/server/adapter.ts, apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.test.ts
The ACP adapter buffers root assistant, reasoning, and subagent assistant text through the coalescer. Tests check update counts, final snapshots, and child-text ordering.
Segment and session flush lifecycle
packages/provider-acp/src/server/adapter.ts, apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.test.ts
The adapter flushes buffered text at segment closure, subagent terminal updates, and session shutdown. Tests cover scope-close flushing and resumed child messages retaining accumulated text.

Priority: ⬆️ High

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant Updates as ACP session updates
  participant Adapter as ACP adapter
  participant Coalescer as ProviderTextDeltaCoalescer
  participant Events as runtime.events
  Updates->>Adapter: Send assistant or reasoning text
  Adapter->>Coalescer: Append delta or set replacement text
  Coalescer-->>Adapter: Provide coalesced text update
  Adapter->>Events: Project text update
  Adapter->>Coalescer: Flush pending text at segment or session close
  Coalescer-->>Adapter: Provide final text update
  Adapter->>Events: Project final text update
Loading

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 19b9d

ACP text coalescing is ready to merge after normal checks; the examined flush paths preserve final text.

Security architecture risk: 🔵 Low · up to 19b9d

The inspected paths preserve text identity and completion ordering without adding execution privileges. Risk is low, with remaining uncertainty around failure and recovery behavior outside the reviewed paths.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The inspected change affects publication of root assistant, reasoning, and child assistant text within ACP session state, plus the shared buffering library used by Claude and Codex. No additional privileged sink was identified in the changed flow; the observed output remains provider event projection.

Trust Boundaries and Controls

  • inferred — Provider-controlled text is buffered and projected using adapter-owned turn and child references. The new library methods do not constitute a remote entrypoint or grant tool authority. Existing generation and teardown checks continue to reject obsolete runtime callbacks.

Resilience and Maintainability Implications

  • observed — Session finalization acquires the callback permit through transport closure, advances the callback generation, and resolves outstanding transport acknowledgements. It then flushes active and carryover text before post-close hooks and runtime teardown. This ordering contains admitted text updates within the closing session.
Pre-merge checks: Passed 4
Passed Passed checks: 4 passed
Check name Status Explanation
Linked Issues check Passed The PR satisfies the coding requirements in [#19461]. packages/provider-acp/src/server/adapter.ts routes root assistant, reasoning, and child text through the shared ProviderTextDeltaCoalescer. Th…
Out of Scope Changes check Passed The changes stay within the linked issue scope. The provider-core coalescer export and implementation support the required shared coalescing behavior. Claude and Codex import updates use that shared l…
Title check Passed The title clearly and concisely describes the primary change: coalescing streamed assistant text snapshots for ACP.
Description check Passed The description explains the problem, implementation, scope, linked issue, verification steps, observed results, limitations, and agent usage. It does not use the template headings exactly and does no…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@maria-rcks

Copy link
Copy Markdown
Collaborator

Note

Written by gpt-6.1-sol on behalf of Maria

Thanks for the ACP streaming work. Under Read this first, right now we are only taking focused bug fixes from outside contributors; this 740-line performance and maintenance rewrite lacks a real-path before/after measurement: https://github.com/pingdotgg/t3code/blob/main/CONTRIBUTING.md#read-this-first. Please preserve the performance problem in an Ideas discussion or return with a smaller bug-fix PR and real-path evidence.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

2 participants