Repository navigation
Conversation
Adds ctx.output_format(format="xml") to request XML from the model, with schema-aligned parsing of the response back into typed values. Covers class/enum/list/map/optional/union/recursion, preamble tolerance, and malformed closing-tag recovery. Shares the output-format backend with TOON. Refs BoundaryML#2409
|
@mvanhorn is attempting to deploy a commit to the Boundary Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds ChangesXML output format support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant JinjaTemplate
participant OutputFormat
participant XmlRenderer
participant Jsonish
participant XmlParser
JinjaTemplate->>OutputFormat: request format="xml"
OutputFormat->>XmlRenderer: render typed XML schema
XmlRenderer-->>JinjaTemplate: return XML prompt
Jsonish->>XmlParser: parse XML-like response
XmlParser-->>Jsonish: return typed Value
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@engine/baml-lib/jinja/src/evaluate_type/types.rs`:
- Around line 362-369: Align the `format` type declaration with the
`output_format()` runtime parser: either update parsing to accept
`Option<String>` and interpret an explicit `none` as the default, or remove
`Type::None` from the union containing the `format` field. Ensure the declared
accepted types cannot produce a value the runtime rejects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 360d9680-a17f-4545-bbb9-2f783be3d737
⛔ Files ignored due to path filters (1)
engine/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
engine/Cargo.tomlengine/baml-lib/jinja-runtime/src/lib.rsengine/baml-lib/jinja-runtime/src/output_format/mod.rsengine/baml-lib/jinja-runtime/src/output_format/types.rsengine/baml-lib/jinja-runtime/src/output_format/xml.rsengine/baml-lib/jinja/src/evaluate_type/test_expr.rsengine/baml-lib/jinja/src/evaluate_type/types.rsengine/baml-lib/jsonish/Cargo.tomlengine/baml-lib/jsonish/src/jsonish/mod.rsengine/baml-lib/jsonish/src/jsonish/xmlish.rsengine/baml-lib/jsonish/src/lib.rsengine/baml-lib/jsonish/src/tests/mod.rsengine/baml-lib/jsonish/src/tests/test_xml.rsfern/03-reference/baml/prompt-syntax/output-format.mdx
Parse the output_format `format` argument as Option<String> so an explicit `none` (allowed by the argument's type union, which also keeps `format` optional) resolves to the default JSON format instead of failing at runtime. Addresses CodeRabbit review.
|
This is the design doc: https://app.notion.com/p/gloochat/XML-Outputs-218bb2d26216806fb4dfef242d150c51?source=copy_link It'd be great to have a ton of tests for this kind of thing (recursive types, arrays, classes, unions). All of these are discussed in the design doc as well |
|
Addressed the review in ba5350a — |
|
Thanks @aaronvg — I'll go through the design doc and make sure the element/attribute mapping and list encoding match it. The PR already covers classes, enums, lists, maps, optionals, unions, and recursion in tests; I'll expand the suite to exercise deeper recursive types and nested arrays/unions per the doc. One thing: the Notion link is gated for me — if there are specific mapping decisions there you want matched exactly (e.g. how unions or nullable fields serialize), a quick note here would help me align precisely. |
Add round-trip and renderer tests for recursive types, class/union arrays, nested arrays, nullable fields, class-valued maps, and malformed-but-recoverable XML. The new nested-array case surfaced a bug where nested XML arrays parsed as empty; xmlish now distinguishes nested <item> containers correctly.
|
Pushed 869ae00 with expanded tests — recursive types, arrays of classes/unions, nested arrays, nullable fields, class-valued maps, and malformed-but-recoverable XML. Adding the nested-array case actually surfaced a real bug (nested XML arrays were parsing as empty), now fixed in |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
engine/baml-lib/jinja-runtime/src/lib.rs (1)
695-695: 📐 Maintainability & Code Quality | 🔵 TrivialRun the engine Rust checks before merge
cargo test --libfromengine;cargo fmtneeds rustfmt in the toolchain.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/baml-lib/jinja-runtime/src/lib.rs` at line 695, Run the engine Rust validation before merging: execute cargo test --lib from the engine workspace and run cargo fmt, ensuring rustfmt is available in the active toolchain.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@engine/baml-lib/jinja-runtime/src/lib.rs`:
- Line 695: Run the engine Rust validation before merging: execute cargo test
--lib from the engine workspace and run cargo fmt, ensuring rustfmt is available
in the active toolchain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 12a680fc-bda7-4b34-b572-92980abaeda4
📒 Files selected for processing (2)
engine/baml-lib/jinja-runtime/src/lib.rsengine/baml-lib/jinja-runtime/src/output_format/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- engine/baml-lib/jinja-runtime/src/output_format/mod.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
engine/baml-lib/jsonish/src/tests/test_xml.rs (1)
306-330: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the total-XML-parse-failure → JSONish fallback path.
The two malformed-XML cases here exercise recoverable XML (partial trees still yield roots). None of the tests exercise the case where
parse_xmlfully fails (roots.is_empty()→bail!) andfrom_strfalls back tojsonish::parse, which is a core contract of this layer per the PR description ("falls back to JSONish parsing on XML errors").test_deserializer!( xml_falls_back_to_jsonish_on_unparseable_xml, XML_TYPES, r#"{"city": "London", "zip": 12345} <unrelated"#, TypeIR::class("Address"), {"city": "London", "zip": 12345} );Verify this input genuinely fails
parse_xml(dangling unclosed tag with no roots) and thatjsonish::parsetolerates the trailing garbage before asserting the exact expected shape.
Based on the upstream contract inengine/baml-lib/jsonish/src/lib.rs: the from_str entrypoint triesjsonish::parse_xmlwhen the input contains<, and if that fails it falls back tojsonish::parse(JSONish parsing) for the same raw payload.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/baml-lib/jsonish/src/tests/test_xml.rs` around lines 306 - 330, Add a test named xml_falls_back_to_jsonish_on_unparseable_xml alongside the existing XML deserializer tests, using the provided JSON object followed by dangling unclosed-tag garbage. Assert it deserializes as Address with the expected city and zip values, thereby covering the parse_xml total-failure path and subsequent jsonish::parse fallback while preserving the exact result shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@engine/baml-lib/jsonish/src/tests/test_xml.rs`:
- Around line 306-330: Add a test named
xml_falls_back_to_jsonish_on_unparseable_xml alongside the existing XML
deserializer tests, using the provided JSON object followed by dangling
unclosed-tag garbage. Assert it deserializes as Address with the expected city
and zip values, thereby covering the parse_xml total-failure path and subsequent
jsonish::parse fallback while preserving the exact result shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 597d8094-ce60-4cd3-bffa-9a6eff6723e7
📒 Files selected for processing (3)
engine/baml-lib/jinja-runtime/src/output_format/types.rsengine/baml-lib/jsonish/src/jsonish/xmlish.rsengine/baml-lib/jsonish/src/tests/test_xml.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- engine/baml-lib/jinja-runtime/src/output_format/types.rs
- engine/baml-lib/jsonish/src/jsonish/xmlish.rs
Match the XML output-format structure to the design doc: nested classes
render inside a <ClassName> wrapper; class arrays use a {ClassName}List
wrapper with <ClassName> elements; primitive/enum/union arrays use
<list><item>; optionals render inline as 'value or null' (via or_splitter
/ render_null_as); class unions render as sibling <ClassName> blocks
separated by a bare OR; recursive classes reference their name inline
with enum/class hoisting and the doc's default prefixes. Parser updated
to accept the new shapes; tests and docs updated to match.
|
Went through the design doc and aligned the XML rendering to it in 94f7de6:
Parser updated to accept the new shapes; 633 tests pass. One question: the doc's canonical arg is |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@engine/baml-lib/jinja-runtime/src/output_format/xml.rs`:
- Around line 651-663: Validate sanitized XML tag names for uniqueness within
each container before rendering, so distinct raw names that collide after
xml_tag_name cannot produce indistinguishable fields. Update the
container-rendering flow around container and xml_tag_name to detect duplicate
sanitized names and reject the container instead of emitting ambiguous XML;
preserve normal rendering for unique names.
- Around line 423-427: Update the catch-all match in the XML rendering function
containing leaf so it is exhaustive: recursively resolve alias variants through
the existing alias-resolution path, and explicitly return the established errors
for unsupported recursive aliases, tuples, arrows, and Top instead of
serializing them with TypeIR::to_string(). Preserve the current scalar fallback
only for genuinely supported scalar variants.
- Around line 218-247: Update the class-rendering flow around the class lookup
and fields loop to render class.description through the existing XML comment
helper before rendering fields. Ensure the description is included for root,
nested, and expanded classes while preserving the existing hoisted-class leaf
behavior and field-description rendering.
- Around line 665-683: Update append_to_field_content to XML-escape the
user-configurable suffix before inserting it into rendered content, ensuring
values containing characters such as & or < remain valid XML while preserving
the existing insertion and fallback behavior.
- Around line 337-348: Update the union rendering flow around
render_union_blocks and container so TypeIR::Map variants emit a typed <map>
root rather than the field tag or generic <value> wrapper. Preserve existing
rendering for non-map variants and null handling, then add round-trip coverage
for map | null and map | class cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6f42c071-9f2e-41e3-9e2e-2bf1f69e9f87
📒 Files selected for processing (6)
engine/baml-lib/jinja-runtime/src/lib.rsengine/baml-lib/jinja-runtime/src/output_format/types.rsengine/baml-lib/jinja-runtime/src/output_format/xml.rsengine/baml-lib/jsonish/src/jsonish/xmlish.rsengine/baml-lib/jsonish/src/tests/test_xml.rsfern/03-reference/baml/prompt-syntax/output-format.mdx
🚧 Files skipped from review as they are similar to previous changes (4)
- engine/baml-lib/jinja-runtime/src/lib.rs
- fern/03-reference/baml/prompt-syntax/output-format.mdx
- engine/baml-lib/jinja-runtime/src/output_format/types.rs
- engine/baml-lib/jsonish/src/jsonish/xmlish.rs
…ustive match, union maps, escaping, class descriptions)
|
Thanks for the thorough pass. Pushed
641 tests pass ( |
Issue Reference
@aaronvg mentioned on #2409 that there's a design doc for an XML output format and asked who'd be interested. This is a first pass — happy to reconcile the element/attribute mapping with whatever the design doc specifies before merge.
Changes
Adds XML as a selectable output format alongside JSON:
{{ ctx.output_format(format="xml") }}renders an XML-shaped schema in the prompt instead of JSON.formatdefaults to"json", so existing prompts are unchanged.formatargument is validated at compile time in the jinja type checker.jsonish) coercion used for JSON, with preamble tolerance, malformed closing-tag recovery, null handling, Unicode aliases, and a depth limit. Existing JSONish behavior for XML-like reasoning/code text is preserved.Rendering lives in
engine/baml-lib/jinja-runtime/src/output_format/(newxml.rs); parsing inengine/baml-lib/jsonish/(newxmlish.rs). Docs infern/03-reference/baml/prompt-syntax/output-format.mdx.Testing
Added XML fixtures covering class/enum/list/map/optional/union/recursion/aliases, preamble tolerance, and malformed closing tags. Local runs:
cargo test -p internal-baml-jinja -p jsonish --lib— 120 + 492 tests passcargo clippy -p internal-baml-jinja -p jsonish --libcleanbaml checkagainst an XML prompt fixture,git diff --checkcleanScreenshots
Simulated demo of the prompt-side contrast (JSON vs XML schema for the same type) and the parse-back to a typed value:
PR Checklist
Additional Notes
Motivation from the issue thread: some models emit more reliable structured output in XML than JSON (the Morph/Warp-Grep guide @yonil7 linked). Branch is aligned with current
canary. AI was used for assistance.Summary by CodeRabbit
ctx.output_format(format=...)to choosenone | "json" | "xml", including XML-structured prompt/output schema rendering.