feat(acp-bridge): stream assistant text in position instead of one end-of-turn block

[Cha0os]

Summary

All of an ACP agent's text arrives in one block at the end of the turn, after every tool call, instead of interleaved with the work it describes.

The cause is an asymmetry in ACPAgent.session_update. Tool calls are emitted as they happen:

elif isinstance(update, ToolCallStart):
    ...
    self._emit_tool_call_event(entry)     # streamed live

Text is not — it is buffered and handed to a callback nothing had subscribed to:

if isinstance(update, AgentMessageChunk):
    self.accumulated_text.append(text)    # buffered for the whole turn
    if self.on_token is not None:
        self.on_token(text)               # nobody was listening

It surfaces only at the end, in _finalize_successful_turn:

response_text = mask("".join(self._client.accumulated_text))
finish_action = FinishAction(message=response_text)

So the whole turn's narration is concatenated — with no separators — into a single FinishAction message. On one conversation I traced, that was 13,253 characters in one event at index 620, following 618 tool-call events. You cannot tell which sentence went with which step, and there is no partial output while the turn runs.

The SDK already supports this and the bridge simply never opted in: Conversation accepts token_callbacks, and LocalConversation forwards them to the agent on every step (agent.step(self, on_event=..., on_token=self._on_token)). This subscribes a relay, buffers the chunks, and flushes each run as a MessageEvent immediately before the next event the bridge forwards.

No UI or server change is required. The web transformer already renders agent-sourced MessageEvents as inline text in event order, and bridge event ingestion passes event_type straight through.

Type of Change

Other — behaviour change in how ACP conversation events are emitted. No API, schema, or UI change.

Why blanking the duplicate finish message is safe

The FinishAction message is the join of the very chunks now sent as MessageEvents, so relaying both shows the entire turn's text twice.

• Nothing server-side consumes that field — there is no FinishAction/finish reference anywhere in services/. It is rendered, not read.
• The transformer only pushes the message when it is truthy, so an empty one renders as nothing.
• The text is still persisted, in the events it was split into. Nothing is lost.
• Only an exact match is dropped. The SDK masks the joined text a second time, which can catch a secret split across two chunks; if that ever makes the two differ, the message is shown as-is rather than discarded.

If you would rather not touch that field at all, the alternative is suppressing it in conversation-to-thread-messages.ts when the turn already produced streamed text. I chose the bridge because it keeps the change in one app and avoids persisting the same text twice — happy to switch if you prefer the presentation-layer fix.

Two things reviewers should know

The token-callback argument type is inconsistent in the SDK. TokenCallbackType is Callable[[LLMStreamChunk], None] (a litellm ModelResponseStream), but the ACP path calls self.on_token(text) with a plain str. _chunk_text handles both so this keeps working whichever way that is reconciled.

Masking. Chunks are masked individually, so a secret split across two of them only becomes matchable once joined — exactly why the SDK re-masks at its own persistence boundary. Segments are a persistence boundary too, so each is re-masked with the conversation's own secret_registry before emission. Residual gap: a secret split across a segment boundary (interrupted by a tool call) still would not match — the same class of limitation the SDK has across turns.

Verification

8 tests added to test_runner.py, 31 pass total:

• text emitted as a MessageEvent before the following event, correctly ordered
• nothing extra emitted when no text was streamed
• duplicate finish message blanked, with the text verified intact across the segments
• a finish message that differs is preserved
• segments re-masked via the conversation registry, including a secret split across two chunks
• bind_masker tolerates a conversation without a registry
• per-turn reset clears both buffers
• _chunk_text accepts a str, a stream chunk, and neither

ruff check src/ and ruff format --check src/ clean.

Checklist

• The change is focused and scoped. One concern: where ACP assistant text is emitted. The dispatch half of _make_event_callback moved into _dispatch_event so the synthetic and real events share one path, keeping the loop/thread handling identical — the two existing callback tests cover it unchanged.
• Related documentation is updated. New "Assistant text arrives in position" section in apps/acp-bridge/README.md.
• New structure or direction is explained clearly. Rationale is in the relay's docstrings and the commit message.
• I avoided unnecessary detail or premature abstraction. One small relay class holding the buffer, the masker, and the duplicate check; no new configuration or public surface.

🤖 Generated with Claude Code

[pullfrog]

Important

One serialization issue to fix before merge: blanking the duplicated FinishAction message re-serializes the payload with json.dumps, which escapes Unicode and changes spacing compared to Pydantic's model_dump_json(). This corrupts non-ASCII text in the same event (e.g., reasoning content) once the message is blanked.

Reviewed changes

This PR wires the ACP bridge's Conversation token callbacks into a new _AssistantTextRelay, flushes buffered assistant text as synthetic MessageEvents before each forwarded SDK event, and blanks the duplicate FinishAction message when it matches the already-streamed text. It also extracts the event-dispatch logic into _dispatch_event so synthetic and SDK events share the same loop/thread handling, and adds focused tests plus a README note.

• apps/acp-bridge/src/paca_acp_bridge/runner.py: adds _chunk_text, _AssistantTextRelay, _strip_duplicated_finish_message, _dispatch_event, and wires token_callbacks into Conversation.
• apps/acp-bridge/tests/test_runner.py: adds 8 tests covering chunk handling, ordering, duplicate blanking, masking, per-turn reset, and registry tolerance.
• apps/acp-bridge/README.md: documents the new streaming behavior.

I verified ruff check src/ and ruff format --check src/ pass. Full pytest could not be run in this environment because installing openhands-sdk timed out.

⚠️ Unicode escaping when blanking the FinishAction payload

json.dumps(data) defaults to ensure_ascii=True and separators=(", ", ": "), while the original event.model_dump_json() keeps Unicode unescaped and omits spaces. When _strip_duplicated_finish_message blanks a finish message, any non-ASCII characters in the same payload (e.g., reasoning_content) will be escaped, and the payload formatting no longer matches the SDK's output. Use json.dumps(data, ensure_ascii=False, separators=(",", ":")), or better, mutate the Pydantic event.action.message before the initial model_dump_json() so the payload is only serialized once.

ℹ️ Nitpicks

• _strip_duplicated_finish_message matches tool_name in (None, "finish"); since SDK ActionEvent.tool_name is a required string, None only matches malformed payloads. Narrowing to "finish" would make the guard's intent clearer.
• _AssistantTextRelay.take_pending swallows masker exceptions at logger.debug and then drops the text; a warning would make masking failures more visible.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏}

[Cha0os]

Thanks — addressed in 59fea55. All three points are handled, though two of them differ from the review's description, so here's what I found.

Serialization: fixed, but it wasn't corrupting anything

The double serialization was real and is gone — the payload is now built once by Pydantic's serializer, with the message cleared on a copy.

But \uXXXX is a lossless encoding of the same string, and agent_conversation_events.payload is jsonb, so Postgres normalizes escaping and spacing away on insert:

'{"t": "café"}'::jsonb = '{"t":"café"}'::jsonb   -- t
'{"t": "café"}'::jsonb ->> 't'                    -- café
'{"a": 1,  "b": 2}'::jsonb = '{"a":1,"b":2}'::jsonb -- t

So no non-ASCII text was being lost, in reasoning_content or anywhere else. The real defect was serializing the same event twice and diverging from the SDK's output for no reason, which is what's fixed.

The suggested fix isn't possible as written

or better, mutate the Pydantic event.action.message before the initial model_dump_json()

Both models are frozen:

ActionEvent frozen: True
FinishAction frozen: True
c.message = ""  ->  ValidationError: Instance is frozen [type=frozen_instance]

In-place mutation would also be wrong independently of that: the SDK keeps this event in conversation history, so writing to it would edit the agent's own record of the turn. 59fea55 uses model_copy(update=...), which works on frozen models and leaves the original intact — with a test asserting the original event is byte-identical after dispatch.

The masking nitpick was the actual bug here

take_pending swallows masker exceptions at logger.debug and then drops the text

It didn't drop the text — that's the part worth flagging. text kept its unmasked value and was emitted:

try:
    text = self._mask(text)
except Exception:
    logger.debug(...)      # falls through, `text` still unmasked

On a masker failure that could persist a secret in cleartext, which is the opposite of how the SDK treats the same failure (fatal to the turn, via _raise_masking_error).

It now fails closed: the segment is dropped and logged at warning. That composes with the duplicate detection — a dropped segment leaves the accumulated text short of the FinishAction message, so that message is no longer seen as a duplicate and is kept. The text still reaches the conversation at the end of the turn, masked by the SDK. No leak, no loss.

tool_name guard

Narrowed to == "finish" as suggested. Confirmed ActionEvent.model_fields["tool_name"].is_required() is True and the annotation is str, so the None arm only ever matched a malformed payload.

Tests

34 pass (31 + 3). The new ones cover non-ASCII preserved unescaped elsewhere in the payload, the original event left unmutated, a masking failure dropping the segment while keeping the finish message, and a non-finish tool_name passing through untouched.

The test double is now a real frozen Pydantic model instead of a hand-rolled stub — the old stub never went through model_copy or Pydantic's serializer, so it would have passed even with the frozen-model problem present.

ruff check src/ and ruff format --check src/ clean. (Noting for the record that pytest couldn't run in the review environment: 34 pass locally against openhands-sdk 1.40.0.)

[pullfrog]

✅ No new issues found.

Reviewed changes

This re-review covers the single new commit since the prior Pullfrog review (738c4fb). It fixes the serialization and masking-visibility concerns raised there.

• Replaced JSON re-serialization with a single Pydantic serialization. _strip_duplicated_finish_message became _event_payload; it now clears the duplicate FinishAction.message on a model_copy and calls event.model_dump_json() once, so non-ASCII fields like reasoning_content are no longer corrupted.
• Preserved Unicode in synthetic MessageEvent payloads. The inline json.dumps({"content": streamed}) now passes ensure_ascii=False.
• Failed closed on masking errors. _AssistantTextRelay.take_pending now logs a warning, drops an unmaskable segment, and leaves the closing FinishAction message intact so the SDK can mask it at the turn boundary.
• Added focused tests for the fixes. New coverage verifies non-ASCII payload preservation, masking-failure fallback, and that only tool_name == "finish" events are rewritten.

All open threads from the prior Pullfrog review were addressed and resolved.

  ^{｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏}

[pikann]

LGTM! Thank you for the contribution! 🚀