fix(flags): a2a streaming - #1889
Conversation
There was a problem hiding this comment.
🟡 Not ready to approve
The new A2A streaming path has correctness gaps (notably text-part format handling and bearer-token streaming behavior) and lacks targeted unit test coverage for the new message/stream branch.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR fixes agentcore invoke --stream for A2A runtimes by plumbing the CLI --stream flag into the A2A invoke path and adding an A2A message/stream (SSE) implementation in the AgentCore client wrapper.
Changes:
- Passes the CLI
--streamoption throughhandleInvoke()intoinvokeA2ARuntime(). - Extends
A2AInvokeOptionswith astream?: booleanflag. - Implements an A2A
message/streamSSE response reader to yield incremental text chunks.
File summaries
| File | Description |
|---|---|
| src/cli/commands/invoke/action.ts | Wires --stream into the A2A invocation options passed to invokeA2ARuntime(). |
| src/cli/aws/agentcore.ts | Adds an A2A streaming option and SSE parsing path for message/stream. |
Review details
Suppressed comments (1)
src/cli/aws/agentcore.ts:1053
- Same as the status-update path: artifact-update streaming currently only recognizes
kind:'text'parts, so streamed output can be silently empty if the runtime usestype:'text'parts (which is already supported in non-streaming parsing).
const text = artifact.parts
.filter(p => p.kind === 'text' && p.text)
.map(p => p.text!)
.join('');
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| // Use message/stream (SSE) for real-time streaming when requested | ||
| if (options.stream) { | ||
| const streamBody = { ...body, method: 'message/stream' }; |
| const text = status.message.parts | ||
| .filter(p => p.kind === 'text' && p.text) | ||
| .map(p => p.text!) | ||
| .join(''); |
| // Use message/stream (SSE) for real-time streaming when requested | ||
| if (options.stream) { | ||
| const streamBody = { ...body, method: 'message/stream' }; | ||
| options.logger?.logSSEEvent(`A2A streaming request: ${JSON.stringify(streamBody)}`); | ||
|
|
|
Working on this. |
…g HTTP agent invocations. Add tests.
There was a problem hiding this comment.
🟡 Not ready to approve
The new A2A streaming generator can drop the final SSE chunk if the stream ends without a trailing newline, which can truncate output.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
src/cli/aws/agentcore.ts:1092
a2aStreamGeneratordrops any trailing partial SSE line when the stream ends without a final\n, becausebufferis never processed after the read loop. This can truncate the last chunk (the other streaming helpers in this file explicitly process the remaining buffer before releasing the reader lock).
}
} finally {
reader.releaseLock();
}
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
There are correctness and test-suite stability issues (decoder flush on streaming, fragile test cleanup, and a missing mocked export that can break existing tests) that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
src/cli/aws/tests/agentcore-a2a-bearer.test.ts:203
afterEachunconditionally callsfetchSpy.mockRestore(), butfetchSpyis only assigned insidemockFetchStream. If a test throws before callingmockFetchStream(or a future test forgets), the cleanup hook will throw and can mask the real failure.
afterEach(() => {
fetchSpy.mockRestore();
vi.clearAllMocks();
});
src/cli/aws/agentcore.ts:1102
TextDecoder.decode(..., { stream: true })should be flushed after the read loop; otherwise a final multi-byte UTF-8 codepoint split across chunks can be dropped/corrupted when the stream ends.
if (buffer) {
yield* parseDataLine(buffer);
}
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| getOrCreatePaymentSession, | ||
| invokeA2ARuntime, | ||
| invokeA2ARuntimeStreaming, | ||
| invokeAgentRuntime, | ||
| invokeAgentRuntimeStreaming, |
There was a problem hiding this comment.
🟡 Not ready to approve
The bug fix is correct in code, but it lacks a unit test that specifically asserts the --stream flag wiring selects invokeA2ARuntimeStreaming() for A2A, which risks regression.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
src/cli/commands/invoke/action.ts:600
- The new A2A
--streambranching is a key bug fix, but there is no unit test coverage in the invoke command tests that verifiesoptions.streamroutes toinvokeA2ARuntimeStreaming()(and notinvokeA2ARuntime()). Adding a targeted test here would prevent future regressions of the flag wiring.
const a2aResult = options.stream
? await invokeA2ARuntimeStreaming(a2aOpts, options.prompt)
: await invokeA2ARuntime(a2aOpts, options.prompt);
src/cli/aws/agentcore.ts:1036
- This A2A SSE parsing / text extraction logic duplicates the existing, tested A2A SSE helpers in src/cli/operations/dev/invoke-a2a.ts (e.g., extractSSEEventText() and its streamedFromStatus de-dupe behavior). Keeping two implementations increases the risk they diverge as the A2A event schema evolves; consider extracting a shared helper module used by both call sites.
async function* a2aStreamGenerator(
reader: ReadableStreamDefaultReader<Uint8Array>
): AsyncGenerator<string, void, unknown> {
const decoder = new TextDecoder();
let buffer = '';
let streamedFromStatus = false;
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟢 Ready to approve
The change is localized, correctly wires the --stream flag for A2A, and includes focused unit tests covering the new routing and streaming behavior.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Description
The
--streamflag was only working for HTTP agent runtimes and not A2A. This PR adds back the missing arguments needed to connect the flag toinvokeA2ARuntime().Related Issue
Closes #1887
Documentation PR
Type of Change
Testing
How have you tested the change?
npm run test:unitandnpm run test:integnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.