Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request enables streaming support for the Express inference format by implementing the ExpressResponseStream class and updating ExpressFormat and ExpressParser to support streaming. It also updates BlockLexer to expose tag detection helpers, and adds corresponding documentation and unit tests. The review feedback points out a bug in ExpressResponseStream.possibleTagStart where returning text.length prematurely prevents the loop from scanning earlier < characters in the chunk if the current one does not match the tag prefix.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds streaming support for the Express inference format, introducing the ExpressResponseStream class to process streamed responses chunk-by-chunk and updating the BlockLexer to find open and close tags. The review feedback highlights a critical bug in BlockLexer.scanBlock where close tags are incorrectly matched inside comments, causing blocks to close prematurely. Additionally, the reviewer noted that the unit test verifying this behavior has a false positive and suggested asserting on the full returned parts to properly catch the issue.
dorny/paths-filter reads a pull request's files from the GitHub API whenever it has a token, and its `token` input defaults to `github.token`. A transient API error then fails the change-detection job, and with it every job that depends on it, on a pull request that changed nothing relevant. Pass an empty token in the three workflows that use the action, so a pull request is read as `git diff pull_request.base.sha..HEAD`, where HEAD is the pull request's merge commit. The action fetches the base commit itself at depth 1, so the shallow checkouts stay as they are. Push and scheduled runs never used the API and are unchanged. The file sets are the same: the merge commit's first parent is `base.sha`, so the diff is the pull request's changes against the base. Checked on a2ui-project#3633, a2ui-project#3634 and a2ui-project#3637: `git diff merge^1..merge` lists exactly the files `GET /pulls/N/files` returns. The filters use no status qualifiers, so the A/M/D statuses git reports cannot change a result either.
6ea5e19 to
510f54b
Compare
nan-yu
left a comment
There was a problem hiding this comment.
Thanks, this unblocks streaming with the default format. I compared the stream against parseResponse on a few more inputs and found three places where they disagree on common model output, plus some smaller points. Details are inline.
Must address
- A block with no open tag is recovered by
parseResponsebut streamed as raw text. - Markdown fences outside the tags are stripped by
parseResponsebut leak into the stream's text. - A forward reference across blocks is accepted by
parseResponsebut throws in the stream.
Should address
4. Errors that are not A2uiError from compile aren't wrapped the way compileBlock wraps them.
5. The open block is rescanned from the start on every chunk.
6. emitText and possibleTagStart duplicate the Direct JSON stream's versions.
| continue; | ||
| } | ||
|
|
||
| const open = this.lexer.findOpenTag(input); |
There was a problem hiding this comment.
Must address: recovery of a block with no open tag is lost.
BlockLexer.tokenize calls tryRecoverUnmatchedClose for a block the model wrote without <a2ui>, but the stream only looks for open and close tags. For this response:
Sure!
```
surface("s1")
root = Text("Hi")
```
</a2ui>
Done.
parseResponse returns text, a2ui, text. The stream returns all of it as text, including the raw DSL and the literal </a2ui>, so the user sees source code instead of UI. Could the stream try the recovery on the held text when it meets a close tag outside a block? If that's out of scope, please list it in KNOWN_GAPS and the README.
There was a problem hiding this comment.
Documented in README.md, KNOWN_GAPS.md, and CHANGELOG.md. Because streaming emits conversational text chunks eagerly before </a2ui> arrives, an Express block where the model omitted <a2ui> cannot be retroactively pulled out of already-emitted text parts; callers needing no-open-tag recovery can use parseResponse().
| return text.length; | ||
| } | ||
|
|
||
| private emitText(parts: ResponsePart[], text: string): void { |
There was a problem hiding this comment.
Must address: fences outside the tags leak into the text.
For Here:\n```\n<a2ui>…</a2ui>\n```\nDone., parseResponse returns the texts "Here:" and "Done.", because addText runs cleanMarkdown. The stream returns "Here:\n```" and "```\nDone.". Models wrap blocks this way often.
The "strips a Markdown fence around the block" test only covers a fence inside the tags. It would help to hold a trailing fence before an open tag and drop a leading fence after a close tag, and to add a response with fences outside the tags to the chunking-invariance test.
There was a problem hiding this comment.
Fixed. ExpressResponseStream now holds a trailing fence line before <a2ui>, drops the fence line immediately preceding <a2ui> and the closing fence line immediately following </a2ui>, and flushes any held prefix on finish() if no <a2ui> follows. Added unit tests covering fenced <a2ui> blocks, 1-char chunking with fences, and conversational code blocks before <a2ui>.
| this.state.transaction(() => { | ||
| const touched = this.state.apply(messages); | ||
| if (isFinal) { | ||
| this.state.checkTopology(touched); |
There was a problem hiding this comment.
Must address: the tree check is stricter than parseResponse.
If one block has root = Column([a]) and a later block defines a, parseResponse accepts the response, because it checks the trees once after every block is applied. The stream throws A2uiIntegrityError on the first block, because each block is checked on its own here.
This matches the Direct JSON stream's rule that every emitted message has been checked, so it may be intended. If so, please say so in the README and CHANGELOG (a stream checks each block on its own; parseResponse checks the response as a whole) and add a test that pins it.
There was a problem hiding this comment.
Documented in README.md, KNOWN_GAPS.md, and CHANGELOG.md, and added a unit test pinning the per-block topology check. Since each </a2ui> block is emitted immediately on close, each block must form a valid component topology on its own (matching how the prompt instructs the model to structure blocks).
(We may revert this decision later if it turns out too restrictive, I guess!)
| this.block = ''; | ||
| const content = this.lexer.cleanBlock(raw); | ||
| if (!content && !isFinal) return; | ||
| const messages = this.parser.compile(content, isFinal); |
There was a problem hiding this comment.
Should address: wrap failures that are not A2uiError.
parseResponse compiles through the private Parser.compileBlock, which turns any error that is not an A2uiError into A2uiCompilationError. This calls parser.compile directly, so an unexpected error from the compiler reaches the caller unwrapped. Could ExpressStreamSource expose the wrapped compile, or could the wrapping move somewhere both paths share?
There was a problem hiding this comment.
Made Parser.compileBlock protected and routed ExpressResponseStream.compileBlock through this.compileBlock(code, surfaceStates) so any non-A2uiError thrown by a custom compiler is wrapped in A2uiCompilationError. Added a unit test verifying this.
| if (this.inBlock) { | ||
| this.block += input; | ||
| input = ''; | ||
| const close = this.lexer.findCloseTag(this.block, 0); |
There was a problem hiding this comment.
Should address: the open block is rescanned from the start on every chunk.
findCloseTag(this.block, 0) runs over the whole block on each feed, so the cost grows with the square of the block's length. With token-sized chunks and a block of a few KB, that's millions of character steps per response.
scanBlock tracks string and comment state, so resuming the scan isn't trivial. A cheap guard would be to rescan only when the new chunk contains <, or when the last scan ended close to one. A resumable scanner like the Direct JSON stream's BlockScanner would remove the issue.
There was a problem hiding this comment.
feed() now skips BlockLexer.findCloseTag(this.block, 0) unless the newly appended chunk contains < or the suffix starting at this.lastTagStart still matches CLOSE_TAG_PREFIX_RE.
| * and the scan goes on to the `<` before it; once a `>` closes a tag the lexer did not | ||
| * match, nothing before it is held. | ||
| */ | ||
| private possibleTagStart(text: string): number { |
There was a problem hiding this comment.
Should address: duplicated logic with DirectJsonResponseStream.
emitText (with runStarted and heldSpace) is a copy of the Direct JSON stream's, and possibleTagStart is a second, different version of blocks.ts#possibleTagStart. Moving the handling of text, whitespace and held tag prefixes into one shared helper would keep the two streams from drifting apart. The fence and recovery fixes above would then apply to both.
There was a problem hiding this comment.
Extracted TextRun into src/parser/text-run.ts and shared it between DirectJsonResponseStream and ExpressResponseStream.
I'll prompt the agent to see if there's more duplication that can be shared across formats.
Express now streams a block at a time, as TypeScript does in #3633: text is emitted as it arrives, and each block once its close tag arrives, compiled and applied whole; a cut-off block is compiled as far as it goes on close. Also bring the blueprints in line with the SDKs on main: - a deleted surface is put back straight into the model, not replayed; - the core pieces a state reverts through, and the core's whole-surface fallback under a deferring ValidationConfig; - a growing updateDataModel is applied again only once it closes; - the Direct JSON prompt: protocol schemas in the base rules, and the schema block delimiters (#3559); compile takes is_final; - the walkthrough's examples argument. Where the SDKs differ now lists Express streaming in Python and Dart, Dart's per-chunk copies, Python's unchecked growing data model, accepts_state in Python, and the other gaps the audit found. Signed-off-by: Nan Yu <nanyu@google.com>
995fa0c to
216cc9f
Compare
|
FYI, @jacobsimionato asked me to update Express's design with streaming support for all the SDKs. This PR is a great start for handling streaming for TS until that lands. I'll be updating #2896 once I've got a design. |
Thanks! We did "need" this so people get a somewhat transparent update when going from directJSON to express without their apps breaking. This is just a stopgap until the real deal comes! Thanks for working on the streaming bits of express! |
nan-yu
left a comment
There was a problem hiding this comment.
Thanks for working through the earlier round. Approving, on the condition that the one inline item (a per-feed transaction) is fixed before merge.
Everything from my previous review is handled:
- No-open-tag recovery and per-block topology are documented as deliberate stream limits (class doc, README, KNOWN_GAPS, CHANGELOG), and per-block topology is pinned by a test.
- Fences around a block are dropped, including when the fence arrives split across chunks.
- Compile errors that aren't
A2uiErrorare wrapped through the now-protectedcompileBlock, with a test. - The close-tag scan is skipped for chunks that can't complete a tag.
TextRunis shared with the Direct JSON stream.
I also compared the stream with parseResponse on 7 typical outputs (fenced blocks with a2ui/express info strings, inline code and code blocks before a block, < in text and in strings, two blocks, a close tag on a comment line), each fed in chunks of 1, 3, 7 and 1000 characters: all 28 runs matched. TS Express unit + conformance tests pass locally (826 passed, 68 skipped).
🤖 Agent-generated
| feed(chunk: string): ResponsePart[] { | ||
| if (this.closed) { | ||
| throw new Error('The response stream is closed.'); | ||
| } | ||
| try { | ||
| return this.read(chunk); | ||
| } catch (e) { | ||
| this.closed = true; |
There was a problem hiding this comment.
Please fix before merge: feed doesn't run the chunk in a transaction, and each block commits its own state.transaction in finishBlock. So a chunk that closes a valid block and then fails on a later block leaves the first block applied, although feed throws and the caller never gets its part.
Repro: one feed with a valid block for s1 followed by a block using Nope("x"). feed throws Component 'Nope' is not defined in the catalog, but state.surfaceIds is still ["s1"], so the state holds a block the renderer never received. That contradicts the class doc ("a stream that throws leaves the state holding the blocks emitted before it") and the ResponseStream rule in the blueprint (#3283).
The Direct JSON stream already does this, so the fix is:
try {
return this.state.transaction(() => this.read(chunk));
} catch (e) {A unit test with two blocks in one chunk, the second invalid, asserting that the state is unchanged after the throw, would pin it.
🤖 Agent-generated
There was a problem hiding this comment.
Ah, what a cool corner case! Let me apply this fix; thanks for noticing it!
Since 0.0.2 Express is the default inference format, and `A2uiRequestProcessor.openStream` threw `Streaming is not supported by ExpressParser` for it (#3598, point 3). Add an interim `ExpressResponseStream`: text outside blocks is emitted as it arrives, less a tail that could still start an open tag, and each block is held until its close tag arrives, then compiled whole, applied to the state inside a transaction with the tree check, and emitted as one part. A response read with `wrapped: false` is one block compiled on `close`, and a response that ends inside a block is compiled as far as it goes, as `parseResponse` does. Applying the statements of an open block as they arrive is left to a streaming Express compiler. `BlockLexer` exposes `findOpenTag`, `findCloseTag`, `cleanBlock` and `openTagName` so the stream shares the one-shot lexer's tag patterns, string and comment skipping, and block cleaning. `ExpressFormat` and `ExpressParser` report `supportsStreaming` true.
A < that is neither the tag nor a prefix of it is text, but an earlier < may still start the tag, as when an open tag's attributes contain one or plain text with a < precedes a partial tag. Continue the scan instead of returning.
…tream The test claimed the block did not close there, and passed only because it looked at the compiled components alone. The lexer closes a block on a close tag written on a single-line comment's line, as response_parser conformance specifies; the test now states that and checks every part.
…the text run - A Markdown fence line right before an open tag or right after a close tag is dropped, as parseResponse drops it. A trailing fence, like a possible tag start, is held until the next chunk settles it. - The open block is scanned for its close tag only when a chunk could have completed one: it holds a `<`, or the last `<` before it still reads as the start of a close tag. - The stream compiles through Parser.compileBlock, so a failure that is not an A2uiError reaches the caller wrapped, as from parseResponse. - TextRun, shared with the Direct JSON stream, replaces the two copies of the text emitter. - README, KNOWN_GAPS and CHANGELOG state that a stream checks each block on its own, and that a block written without its open tag, which parseResponse recovers, is read as text.
216cc9f to
335e0ea
Compare
|
Actually, I'm going to abandon/close this in favor of greg's full implementation, which is here: It doesn't make much sense to land an interim version and then a better one immediately afterwards. |
|
(Unstacked) |
Since 0.0.2 Express is the default inference format, but
A2uiRequestProcessor.openStreamthrewStreaming is not supported by ExpressParserfor it (#3598, point 3). This is the interim fix so 0.0.3 can keep Express as the default while a streaming Express compiler is designed.What changes
ExpressResponseStream. Text outside blocks is emitted as it arrives, less a tail that could still start an open tag. Each block is held until its close tag arrives, then compiled whole, applied to the state in a transaction together with the tree check, and emitted as onea2uipart. A response read withwrapped: falseis one block compiled onclose. A response that ends inside a block is compiled as far as it goes, asparseResponsedoes for a cut-off block.ExpressParser.openStreamreturns it;ExpressParser.supportsStreamingandExpressFormat.supportsStreamingare nowtrue.BlockLexerexposesfindOpenTag,findCloseTag,cleanBlockandopenTagNameso the stream shares the one-shot lexer's tag patterns, string/comment skipping and block cleaning instead of duplicating them.What does not change
The statements of an open block are not applied as they arrive. That needs a streaming Express compiler (#2896) and is deliberately out of scope here.
Tests
12 new unit tests in
tests/unit/inference-formats/express/response-stream.test.ts(text/block emission, chunking invariance againstparseResponse, held tag prefixes, comment skipping, fence stripping, state application,wrapped: false, rejected block keeps prior state, compile error, cut-off block, double close). Full suite: 1120 passed.Part of #3598.
Pre-launch Checklist
One time:
For this PR:
If you need help, consider asking for advice on the discussion board.
Stack created with GitHub Stacks CLI • Give Feedback 💬