ag-ui-protocol / ag-ui-protocol/ag-ui

[Bug]: a stream that ends without RUN_FINISHED/RUN_ERROR resolves as success (truncated run is indistinguishable from a completed one)

Abierto
#2,300 3 comentarios 0 reacciones 0 asignados Ver en GitHub
Lenguaje dominante
Python
Estrellas
15.9k
Forks
1.4k
Merge medio
1 d 17 h
PR fusionados (30 d)
163

Descripción

### Pre-flight Checklist

- [x] I have searched existing issues and this hasn't been reported yet.
- [x] I am using the **latest** version AG-UI (`@ag-ui/client` 0.0.57).

### Summary

The spec says a run's terminal event is mandatory:

> The `RunStarted` and either `RunFinished` or `RunError` events are mandatory, forming the boundaries of an agent run.

`verifyEvents` enforces a lot around those boundaries, but it never checks that a terminator arrived at all. When a stream ends mid-run (connection drop, LB idle timeout, server OOM, pod eviction), `runAgent()` **resolves successfully**, no lifecycle hook fires, and the partial assistant message is committed to `agent.messages` as if complete.

So a truncated run is byte-for-byte indistinguishable from a successful one, and the truncation point can land mid-token.

### Repro

Real `HttpAgent`, real SSE bytes, injected `fetch` so there is no server to run:

```js
import { HttpAgent } from '@ag-ui/client'

const FRAMES = [
{ type: 'RUN_STARTED', threadId: 'thread_1', runId: 'run_1' },
{ type: 'TEXT_MESSAGE_START', messageId: 'msg_1', role: 'assistant' },
{ type: 'TEXT_MESSAGE_CONTENT', messageId: 'msg_1', delta: 'Transferring $50,0' },
// socket dies here: no RUN_FINISHED, no RUN_ERROR
]

const fetchImpl = async () =>
new Response(
new ReadableStream({
start(c) {
const enc = new TextEncoder()
for (const f of FRAMES) c.enqueue(enc.encode(`data: ${JSON.stringify(f)}\n\n`))
c.close()
},
}),
{ status: 200, headers: { 'content-type': 'text/event-stream' } },
)

const agent = new HttpAgent({ url: 'http://example.test/agent', fetch: fetchImpl })

let flagged = false
const result = await agent.runAgent({}, {
onRunFailed: () => { flagged = true },
onRunErrorEvent: () => { flagged = true },
})

console.log('resolved: ', JSON.stringify(result))
console.log('flagged: ', flagged)
console.log('messages: ', JSON.stringify(agent.messages))
```

Output:

```
resolved: {"newMessages":[{"id":"msg_1","role":"assistant","content":"Transferring $50,0"}]}
flagged: false
messages: [{"id":"msg_1","role":"assistant","content":"Transferring $50,0"}]
```

### The asymmetry

Same harness, six streams. Only the middle two are rejected, and they are the *stricter* violations:

| stream | valid? | result |
|---|---|---|
| `RUN_STARTED, MSG_START, MSG_CONTENT, MSG_END, RUN_FINISHED` | yes | resolves |
| `RUN_STARTED, MSG_START, MSG_CONTENT, RUN_FINISHED` (msg left open) | no | **throws** `Cannot send 'RUN_FINISHED' while text messages are still active: m` |
| `... RUN_FINISHED, RUN_FINISHED` (duplicate terminator) | no | **throws** `The run has already finished with 'RUN_FINISHED'` |
| `RUN_STARTED, MSG_START, MSG_CONTENT` (truncated) | no | resolves, nothing flagged |
| `RUN_STARTED, MSG_START, MSG_CONTENT, MSG_END` (no terminator) | no | resolves, nothing flagged |
| `RUN_STARTED` (nothing else) | no | resolves, nothing flagged |

A run with *two* terminators is caught. A run with *zero* is not. That looks like an oversight rather than a deliberate tolerance, especially since zero is the case the network produces on its own.

### Cause

`verifyEvents` is a `mergeMap` state machine over arriving events. It tracks `runStarted` / `runFinished` / `runError` and rejects bad transitions, but the pipeline has no `finalize`/completion handler, so stream completion is never inspected. `lastValueFrom` then resolves normally and `runAgent()` returns `{ result, newMessages }`.

Related but distinct: #1892 was the mirror of this (an ADK integration emitting `RUN_ERROR` **then** `RUN_FINISHED`, correctly rejected by the client). That direction is guarded; this one isn't.

Worth noting for cross-SDK parity: #1327 (`validate_sequence` for the Python SDK) ports the same design, so it inherits the same blind spot. Cheaper to settle the semantics once, here, before both SDKs ship it.

### Suggested fix

Assert the invariant on stream completion in `verifyEvents`: if the source completes while `runStarted && !runFinished && !runError`, error the stream (an `AGUIError` naming the incomplete run, mirroring the existing message style). That surfaces truncation through the channel apps already handle, needs no wire change, and is roughly the same shape as the existing active-message/active-step checks that already run at `RUN_FINISHED`.

Two details worth deciding explicitly:

1. **Should this be opt-out?** A strict-by-default throw is a behaviour change for anyone currently (unknowingly) relying on partial results. If that is a concern, a config flag defaulting to strict, or routing it through `onRunFailed` rather than a rejection, both work. I would lean strict-by-default since the current behaviour silently loses data.
2. **Streams with no `RUN_STARTED` at all** (an immediately closed 200) currently resolve too. Same check covers it if the condition is "started but unterminated"; a separate rule is needed if an empty stream should also fail.

Happy to open a PR with the `finalize` check plus tests for the six cases above if you'd like it in that form.

### Environment

- `@ag-ui/client` 0.0.57 (latest on npm at time of filing)
- `@ag-ui/core` 0.0.57
- Bun 1.3.14, macOS arm64 (nothing runtime-specific; the pipeline is the same under Node)

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.