feat: preserve accumulated schema and desktop improvements - #787
Conversation
Running the verification loop across every provider instead of only the route I had tested live, which is what the review asked for, found that my own #713 fix had silently reopened #543 about ten minutes earlier. Unsupported `format` values were stripped only inside `strict_normalize_schema`. That was invisible while every well-formed catalog took the strict path. #713 makes a typeless property force strict off, so those catalogs now skip the only place `format: "uri"` was removed and send it to a validator that rejects it. Stripping moves to `openai_compatible_schema`, which runs unconditionally. New coverage, both mutation-verified rather than assumed: - `every_provider_sends_clean_schemas` runs one schema carrying the trigger from every issue in this class (#446, #495, #543, #687, #713, #754) through each provider's real request builder and asserts on what would reach the wire. Behavioral, so it holds whether a provider reaches the dialect engine or its own sanitizer. This is the test that caught the regression above. - `recovery_coverage` asks the recovery question of every provider rather than the single route the live experiment covered. It also pins which runtimes call `recover_from_error`, reading their sources so the claim cannot drift from the code. The wiring check took two attempts, and the first one is why it is per-runtime now: keyed by dialect, it stayed green when I unwired the native Gemini route, because the Antigravity runtime also dispatches to the `gemini` dialect and kept it looking covered. Unwiring that route now fails with the exact file and call count. Recorded gaps rather than hidden ones: OpenAI, OpenRouter and Anthropic have dialects in the registry but their request builders still use their own older sanitizers, and their runtimes have no recovery wiring. Both facts are now asserted, so the migration is bounded work instead of a sweep passing over code nothing executes.
…itizer
Closes the gap the cross-provider loop exposed: OpenRouter had a dialect
in the registry that the sweep validated while its request builder ran a
separate hand-written sanitizer, so the swept code was never executed.
Diffed the two implementations before switching rather than assuming they
agreed. Identical on a bare `{"type": "object"}`, an empty schema, and a
top-level combiner; on a hostile schema the engine additionally strips
`propertyNames` and `uniqueItems`, which the old version forwarded to the
same Anthropic-family and strict upstreams that reject them. So the
migration is a strict improvement, not a wash, and all 26 existing
OpenRouter tests pass against it unchanged.
Removes `flatten_top_level_combinators`, now dead.
Adds a pin for which provider builders reach the engine, mutation-verified
by claiming OpenAI uses it and watching the test name the file. OpenAI and
Anthropic remain on their own sanitizers: both work and both have their
wire output pinned by `every_provider_sends_clean_schemas`, and OpenAI
additionally owns strict-eligibility logic (#711, #713) with no dialect
equivalent yet. That is duplication to remove later, now tracked in code
instead of a commit message.
…xposed
Second of the three providers whose registry entry the sweep validated
while its request builder ran a private sanitizer.
Diffing the two before switching (as with OpenRouter) found the dialect
was wrong, not the provider: `anthropic_input_schema` guaranteed an object
schema with a `properties` map, coercing a bare `{"type":"object"}` and
even a non-object schema, because the API rejects both. The ANTHROPIC
dialect did not, so migrating as-is would have introduced the failure the
provider code was written to prevent. Fixed by giving the dialect
`require_properties_on_objects`, after which 5 of 6 probe schemas are
byte-identical and the sixth differs only by an added empty `properties`
on a nested object, which is a JSON Schema no-op Anthropic accepts.
This is the second time diffing first paid for itself, so it is worth
stating as the rule: a shared engine is only safe to adopt where it has
been shown to match or beat what it replaces, per case, not per intention.
Adds Anthropic to the cross-provider wire test, which had covered every
other route: no top-level combiner survives, every branch's properties are
still advertised, a branch-only `required` is not promoted into a demand
the merged object cannot express, and a no-argument tool still gets the
object shape the API requires.
The engine pin caught this migration and failed until updated, which is
what it is for. OpenAI is now the last holdout, deliberately: its path
also decides strict eligibility (#711, #713) and runs a separate strict
normalization, neither of which the engine can express yet.
Answers the question this work started from, rather than leaving it open: could a keyword nobody has seen still take a provider down? For OpenAI, yes. Probing it directly showed `someFutureKeyword` forwarded verbatim into the request, because that path was still an inverted deny-list. It was the last one. Diffing the dialect against it first, as with the other two migrations, found two places the dialect was the weaker one, both fixed here rather than accepted: - OpenAI's sanitizer merged `allOf` branch properties into the parent. The dialect kept the `allOf`, which OpenAI accepts syntactically without intersecting, so a schema whose properties live only in branches would have advertised none of them. Added `merge_all_of_branches`. - That merge then had to adopt a branch's `type` when the parent declares none, or the schema comes out typeless once the `allOf` is gone. After both, 9 of 10 probe schemas are byte-identical and the tenth differs only by the novel keyword now being dropped. Strict eligibility and strict normalization stay in jcode-provider-core: they are OpenAI-specific and have no dialect equivalent. Adds the test the system exists for: an invented keyword that appears in no list, issue, or provider doc must reach none of the five providers. It would have failed on every one of them before this work. Also fixes a real defect this uncovered in my own earlier test code. An early version of the quirk-store test hook used a process-global env var, and because tests run in parallel one of them saw it unset and persisted a learned `minItems` rejection into the real ~/.jcode/schema-quirks.json, after which every OpenAI request on this machine silently dropped `minItems`. That is how it was found: a test asserting `minItems` survives began failing for reasons nothing in the diff explained. The store now returns no path at all under cfg(test)/test-support, so a test that forgets to isolate cannot read or write the real file instead of merely being expected not to.
Greptile SummaryThis PR adds shared provider schema cleanup and carries forward desktop2 workspace and rendering improvements. The main changes are:
Confidence Score: 4/5Mostly safe to merge after the inline math reveal bug is fixed. One contained issue affects desktop2 streaming display for inline math. Schema dialect and workspace changes reviewed without another concrete bug. Files Needing Attention: crates/jcode-desktop2/src/scene.rs; crates/jcode-desktop2/src/transcript.rs
What T-Rex did
|
| Filename | Overview |
|---|---|
| crates/jcode-desktop2/scripts/reapply_image_paste.py | Adds an idempotent helper for reapplying desktop2 image-paste wiring; no specific issues found. |
| crates/jcode-desktop2/src/scene.rs | Draws native inline math boxes in transcript blocks, but currently reveals inline formulas all at once during streaming. |
| crates/jcode-desktop2/src/scene_workspace.rs | Renders workspace pages with rounded clipping, borders, and row-slide support; no specific issues found. |
| crates/jcode-desktop2/src/transcript.rs | Tracks inline math formulas in laid transcript blocks and includes their glyph counts for reveal accounting. |
| crates/jcode-desktop2/src/workspace.rs | Extends workspace geometry to grouped rows and vertical transitions; no specific issues found. |
| crates/jcode-provider-core/src/openai_schema.rs | Delegates OpenAI-compatible schema normalization to the shared dialect engine while retaining strict-mode checks. |
| crates/jcode-provider-openai-runtime/src/openai_provider_impl.rs | Learns OpenAI tool-schema rejections through the shared schema dialect recovery path; no specific issues found. |
| crates/jcode-schema-dialect/src/dialect.rs | Implements allow-list based schema normalization, combiner transforms, and learned quirk application; no specific issues found. |
| crates/jcode-schema-dialect/src/lib.rs | Adds normalization/recovery entry points and schema-rejection learning; no specific issues found. |
Sequence Diagram
sequenceDiagram
participant Provider as Provider runtime
participant Dialect as jcode-schema-dialect
participant Quirks as Learned quirks store
participant API as Model provider API
Provider->>Dialect: normalize(tool schema, provider spec)
Dialect->>Quirks: learned_for(spec.id)
Quirks-->>Dialect: rejected keywords/formats
Dialect-->>Provider: provider-compatible schema
Provider->>API: send request with cleaned tools
API-->>Provider: success or schema rejection
alt schema rejection names construct
Provider->>Dialect: learn_from_error(error, spec)
Dialect->>Quirks: record_keyword/record_format
Dialect-->>Provider: explanation for user/logs
else non-schema or unrecoverable
Dialect-->>Provider: pass through or hint
end
Prompt To Fix All With AI
### Issue 1
crates/jcode-desktop2/src/scene.rs:1411
**Preserve reveal accounting**
`block.glyphs` now includes `inline_math` glyphs, but the draw path always renders each inline formula with `f64::INFINITY`. During streaming, as soon as the surrounding block has any reveal budget, every inline formula in that visible block is painted completely, while `drawn_glyphs += block.glyphs` still charges those math glyphs to the reveal budget. This breaks the existing per-glyph reveal contract for messages containing inline math.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat: preserve accumulated schema and de..." | Re-trigger Greptile
| ), | ||
| theme.text, | ||
| scale, | ||
| f64::INFINITY, |
There was a problem hiding this comment.
Preserve reveal accounting
block.glyphs now includes inline_math glyphs, but the draw path always renders each inline formula with f64::INFINITY. During streaming, as soon as the surrounding block has any reveal budget, every inline formula in that visible block is painted completely, while drawn_glyphs += block.glyphs still charges those math glyphs to the reveal budget. This breaks the existing per-glyph reveal contract for messages containing inline math.
Artifacts
Repro: Rust harness source for inline math reveal accounting
- Evidence file captured while the check ran.
Repro: cargo run output showing complete inline formula draw under partial reveal
- The full command output behind this check.
Ran code and verified through T-Rex
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/jcode-desktop2/src/scene.rs
Line: 1411
Comment:
**Preserve reveal accounting**
`block.glyphs` now includes `inline_math` glyphs, but the draw path always renders each inline formula with `f64::INFINITY`. During streaming, as soon as the surrounding block has any reveal budget, every inline formula in that visible block is painted completely, while `drawn_glyphs += block.glyphs` still charges those math glyphs to the reveal budget. This breaks the existing per-glyph reveal contract for messages containing inline math.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Summary
Verification
cargo fmt --all -- --checkcargo test --profile selfdev -p jcode-schema-dialect -p jcode-render-core -p jcode-provider-core -p jcode-provider-openai-runtimecargo check --profile selfdev -p jcode-desktop2--- — Jcode agent (automated triage), on behalf of @1jehuang