Replace hand-rolled Lua/JSON converters with a serde bridge - #108
Merged
Conversation
Single module with pinned serialize and deserialize options, replacing what will be four hand-rolled converters. Options are fixed here rather than per call site so payloads round-trip identically regardless of which API carried them.
Events reached Lua by building Lua source and load()ing it. Object keys escaped quotes but not backslashes, so a key ending in a backslash produced a malformed literal: the eval failed and the plugin silently received a raw JSON string instead of a table. Drop the string fallback with the codegen. A conversion failure now logs and drops the event rather than handing plugins an unexpected type.
Replaces lua_value_to_json across publish, settings sections, plugin queries, docked views, and form dialogs. Functions and cyclic tables now raise instead of silently serializing as null or recursing until the stack overflows.
Deletes the last two hand-rolled JSON to Lua converters. Non-object responses now reach plugins as nil rather than an empty table.
The previous helper copied only scalars, so nested objects and arrays in host responses were silently dropped before reaching plugins.
Review found the session merge test only exercised merge_json_into_table directly, missing the JSON parsing, match guard, and post-merge default-fill at the session.current()/exec_active() call sites, and that the "defaults survive" assertion never covered a colliding key. Add a call-site test driving session.current() through a mock HostApi, and a colliding-key assertion to the existing merge test. Also narrow the plugin-sdk.md note to name session.current() and session.exec_active() explicitly rather than reading as if it applied to the whole session table.
Verified no hand-rolled converters remain (grep for lua_value_to_json/json_to_lua_table/json_to_lua_value/json_to_lua_literal/ json_value_to_lua_literal/set_lua_table_from_json_map returns nothing) and that SerializeOptions/DeserializeOptions are defined only in lua/convert.rs. Cross-checked every app.*, ui.*, session.*, and net.* function registered in lua/api/*.rs against docs/plugin-sdk.md in both directions: all are documented, and two drift items surfaced: - ui.panel_html was implemented and listed in the Lua Signatures block but missing from the Panel Widget Functions reference table; added it. - app.query_plugin's conversion note said "publishing" (copy-pasted from app.publish above it) when query_plugin sends an RPC call, not a bus publish; reworded. Also added a Lua->JSON test for float preservation in convert.rs. The replacement suite covered float/integer fidelity only for JSON->Lua (number_kinds_are_preserved); the deleted lua_value_to_json_primitives test was the only Lua->JSON float assertion, and nothing replaced it.
Add a real call-site test for ui.form's nil-on-non-object-response behavior via a mock HostApi, and a test covering dispatch_event_json_raw's malformed-JSON drop path — both were previously untested despite being reachable from frontend/plugin input. Delete two ui.rs tests that duplicated convert.rs's own coverage while claiming call-site names. Document the raises-a-Lua- error-on-functions-or-cycles behavior for register_settings_section, open_docked_view, and form, which shared the same conversion bridge as publish/query_plugin but weren't mentioned in the docs. Drop a redundant convert import in ui.rs in favor of the module-qualified call style already used nearby. Hand-fix the five rustfmt hunks this branch introduced, without touching the package's pre-existing formatting debt.
Brings in the CI pipeline fixes and the markdown preview work. No conflicts: this branch only touches termlab_plugin's Lua/JSON conversion and docs/plugin-sdk.md, neither of which main changed. Full workspace suite green after the merge, including the 93 termlab_plugin tests covering the new bridge.
Brings in the plugin HTML sanitizer from #107. No conflicts: that branch's docs/plugin-sdk.md edits cover the html widget's sanitization allowlist, while this branch's cover payload conversion behavior for app.publish, app.query_plugin, app.register_settings_section, ui.form, ui.open_docked_view, session.current and session.exec_active. Different sections, no contradiction. Full workspace suite green after the merge, including the 93 termlab_plugin tests covering the new bridge.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Replaces four hand-rolled Lua↔JSON converters in
termlab_pluginwith a single serde-backed bridge (crates/termlab_plugin/src/lua/convert.rs).The one that mattered: plugin events reached Lua by building Lua source text from JSON and
load()ing it. Object keys escaped"but not\, so a key ending in a backslash produced a malformed literal — the eval failed and the plugin silently received a raw JSON string where it expected a table.bus_event_key_with_backslash_is_delivered_intactis a regression test against exactly that.The other three:
lua_value_to_json(Lua→JSON) — used byapp.publish,register_settings_section,query_plugin,ui.open_docked_view, form dialogsjson_to_lua_table/json_to_lua_value(JSON→Lua) — docked-view and form dialog responsesset_lua_table_from_json_map— merged host responses into a Lua table copying only scalars, so nested objects and arrays were silently dropped fromsession.current()andsession.exec_active()That fourth one wasn't in the original design; it turned up during planning.
Behavior changes (all deliberate, all documented)
null{}nilEvery in-tree plugin and example was traced against these: all pass plain data with no functions or cycles, and the docked-view example already guards with
if type(result) ~= "table".Conventions, verified not assumed
The serde options are pinned in one place.
serialize_unit_to_null(false)andserialize_none_to_null(false)are load-bearing — mlua's default maps JSONnullto a lightuserdata sentinel that compares unequal tonil, which would have quietly broken everyif x == nilin plugin code. These behaviors were confirmed with a characterization spike against mlua 0.10 before the code was written, and each has a test.Test plan
cargo test --workspacetermlab_plugingoes 77 → 93 tests. Includes call-site tests drivingsession.current()andui.form()through a mockHostApi, not just the bridge underneath them.Notes for review
panel_htmldoc row saying "Raw HTML rendered in a Shadow DOM" — accurate here, stale next to the sanitization section Sanitize plugin-supplied HTML and validate icon names #107 adds. Whoever merges second should reconcile the wording.handle_querystill handson_querya hand-serialized JSON string and discards a malformed reply silently. Out of scope here; it's the last hand-rolled JSON edge in the Lua runtime.🤖 Generated with Claude Code